projects / postgresql.git / commitdiff
summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: ef1811a)
Fix assertion failure during decoding from synced slots.
authorAmit Kapila <akapila@postgresql.org>
Tue, 29 Apr 2025 07:22:05 +0000 (12:52 +0530)
committerAmit Kapila <akapila@postgresql.org>
Tue, 29 Apr 2025 07:22:05 +0000 (12:52 +0530)
The slot synchronization skips updating the confirmed_flush LSN of the
local slot if the local slot has a newer catalog_xmin or restart_lsn, but
still allows updating the two_phase and two_phase_at fields of the slot.
This opens up a window for the prepared transactions between old
confirmed_flush LSN and two_phase_at to unexpectedly get decoded and sent
to the downstream after promotion. Then, while decoding the commit
prepared the assert will fail, which expects that the prepare hasn't been
sent to the downstream.

The fix is to skip updating the other slot fields when we are skipping to
update the confirmed_flush LSN of the slot.

We didn't backpatch this commit as two_phase_at was not synced in back
branches, which means prepared transactions won't be unexpectedly sent to
downstream.

We discovered this problem while analyzing BF failure reported in the
discussion link.

Reliably reproducing this issue without a debugger is difficult. Given
its rarity, adding specific injection point to test it doesn't seem
worthwhile, so we won't be adding a dedicated test case.

Author: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/OS0PR01MB5716B44052000EB91EFAE60E94BC2@OS0PR01MB5716.jpnprd01.prod.outlook.com

src/backend/replication/logical/slotsync.c patch | blob | blame | history

diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index e22d41891e6867030a7d8400f9e59c08c9fae284..656e66e0ae0a10888da111b22835bfdd471f5d42 100644 (file)
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -196,14 +196,14 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid,
         * restart_lsn or the initial xmin_horizon computed for the local slot
         * is ahead of the remote slot.
         *
-        * If the slot is persistent, restart_lsn of the synced slot could
-        * still be ahead of the remote slot. Since we use slot advance
-        * functionality to keep snapbuild/slot updated, it is possible that
-        * the restart_lsn is advanced to a later position than it has on the
-        * primary. This can happen when slot advancing machinery finds
-        * running xacts record after reaching the consistent state at a later
-        * point than the primary where it serializes the snapshot and updates
-        * the restart_lsn.
+        * If the slot is persistent, both restart_lsn and catalog_xmin of the
+        * synced slot could still be ahead of the remote slot. Since we use
+        * slot advance functionality to keep snapbuild/slot updated, it is
+        * possible that the restart_lsn and catalog_xmin are advanced to a
+        * later position than it has on the primary. This can happen when
+        * slot advancing machinery finds running xacts record after reaching
+        * the consistent state at a later point than the primary where it
+        * serializes the snapshot and updates the restart_lsn.
         *
         * We LOG the message if the slot is temporary as it can help the user
         * to understand why the slot is not sync-ready. In the case of a
@@ -221,16 +221,25 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid,
 
        if (remote_slot_precedes)
            *remote_slot_precedes = true;
+
+       /*
+        * Skip updating the configuration. This is required to avoid syncing
+        * two_phase_at without syncing confirmed_lsn. Otherwise, the prepared
+        * transaction between old confirmed_lsn and two_phase_at will
+        * unexpectedly get decoded and sent to the downstream after
+        * promotion. See comments in ReorderBufferFinishPrepared.
+        */
+       return false;
    }
 
    /*
     * Attempt to sync LSNs and xmins only if remote slot is ahead of local
     * slot.
     */
-   else if (remote_slot->confirmed_lsn > slot->data.confirmed_flush ||
-            remote_slot->restart_lsn > slot->data.restart_lsn ||
-            TransactionIdFollows(remote_slot->catalog_xmin,
-                                 slot->data.catalog_xmin))
+   if (remote_slot->confirmed_lsn > slot->data.confirmed_flush ||
+       remote_slot->restart_lsn > slot->data.restart_lsn ||
+       TransactionIdFollows(remote_slot->catalog_xmin,
+                            slot->data.catalog_xmin))
    {
        /*
         * We can't directly copy the remote slot's LSN or xmin unless there
@@ -294,6 +303,12 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid,
        SpinLockRelease(&slot->mutex);
 
        updated_config = true;
+
+       /*
+        * Ensure that there is no risk of sending prepared transactions
+        * unexpectedly after the promotion.
+        */
+       Assert(slot->data.two_phase_at <= slot->data.confirmed_flush);
    }
 
    /*
This is the main PostgreSQL git repository.