Обсуждение: Fix logical decoding not track transaction during SNAPBUILD_BUILDING_SNAPSHOT

Hi Haiyang,

Thank you for your summary.

One important thing is that we must not skip any call to ReorderBufferXidSetCatalogChanges() (direct or indirect)
duringfast-forwarding or building snapshot, because the historic snapshot only tracks txns with catalog changes, the
v6-0004seems to skip it in xact_decode(). 

Here is a related bug:

https://www.postgresql.org/message-id/flat/tencent_3A071B760AA1A38540B57F297332B7781C08%40qq.com

--
Regards,
ChangAo Chen

Hi Haiyang,

> v6-0004 only skip the transaction commited during START state and precedeing next_phase_at
> (set when changing to BUILDING_SNAPSHOT state) during BUILDING_SNAPSHOT. Those transactions
> are always useless no matter in fast forward or not. Plaese recheck v6-0004 again.

Yeah, you're right. What's useful for building the snapshot and we currently don't track are the txns
start after BUILDING_SNAPSHOT and commit before FULL_SNAPSHOT, their xids all >= next_phase_at
during BUILDING_SNAPSHOT.

--
Regards,
ChangAo Chen

Dear Li,

Sorry, can you evaluate bit more why the 0003 patch is enough for the fix? E.g.,
apart from 0001, 0003 adds invalidation message to the transaction (or immediately
invalidate) even if we do not have the consistent snapshot yet.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Saturday, November 22, 2025 5:28 PM ocean_li_996 <ocean_li_996@163.com> wrote:
> # Problem description
> 
> When in the BUILDING_SNAPSHOT state, the snapshot builder does not track the
> status of any transaction. It can lead to missing transaction states when: --
> The transaction commits before the builder reaches FULL_SNAPSHOT state, and --
> The transaction's xid is greater than or equal to builder->xmin when the builder
>    reaches FULL_SNAPSHOT state.
> 
> Once in FULL_SNAPSHOT state, the builder constructs a base snapshot using
> incomplete transaction state information. This results in an incorrect base
> snapshot, which can cause unpredictable behavior during subsequent decoding. The
> case provided in v6-0002 attachment reproduces the issue (provided by ChangAo
> Chen).

I agree that this is clearly a bug and should be fixed.

> 
> # Code-level analysis
> 
> SnapBuildCommitTxn does consider transaction processing during the
> BUILDING_SNAPSHOT state. However, it is only called from xact_decode ->
> DecodeCommit. xact_decode does not process any xact record when snapshot builder
> have not yet reached the FULL_SNAPSHOT state, meaning those commits are ignored.
> Similarly, other functions marking transaction having catalog changes (e.g.,
> heap2_decode) also do not handle records before reaching the FULL_SNAPSHOT
> state.

The analysis looks correct to me.

> # Possible fixes
> 
> 1. Replace snapshot at the time we reach CONSISTENT state.
> 
> Ajin Cherian in [4] and my initial thought was that although the snapshot at
> FULL_SNAPSHOT state might be wrong, the snapshot at CONSISTENT state is
> guaranteed to be correct. Since decoding always starts after reaching
> CONSISTENT state, we could update both the reorder buffer and the builder
> snapshot with the one captured at CONSISTENT state. However, IMUC, this would
> still cause changes generated before CONSISTENT to carry a wrong snapshot (see
> SnapBuildDistributeSnapshotAndInval).
> 
> 2. Track transactions during BUILDING_SNAPSHOT state for snapshot builder If the
> builder does not track transactions in BUILDING_SNAPSHOT state, then we make it
> track them.
> 
> 1) ChangAo Chen in v6-0001 attachment provided a fix, already reviewed by
> several people (including me). Bertrand Drouvot in [5] considered the logic a
> bit messy. And I prefer we should make the behavior of snapshot building similar
> in both BUILDING_SNAPSHOT and FULL_SNAPSHOT states, except in cases where a base
> snapshot is needed.
> 
> 2) Based on v6-0001, I have provided a minimal fix in v6-0003 (not yet
> reviewed). AFAICS, it resolves the problem, though it records additional useless
> information in the reorder buffer during BUILDING_SNAPSHOT state (which is
> discarded later). This increases memory usage and slightly impacts performance.
> But since snapshot building is infrequent, I consider this acceptable.
> 
> 3) I have also prepared a cleaner and more efficient fix in v6-0004 than
> v6-0003, albeit more complex (similar to v6-0001). Provided as an alternative
> reference.

I also thought about how to fix this issue, and my draft idea is similar to what
your v6-0004 implements, e.g., track transactions during BUILDING state but also
avoid unnecessary cost during decoding. So, I think it's on the right track.

Best Regard,
Hou zj

Hi all,

I think the only ugly code in v6-0001 is:

```
     if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT)
+    {
+        /*
+         * If we are building snapshot and the xlog means a catalog
+         * change, we need to mark it in the reorder buffer.
+         *
+         * Now only XLOG_HEAP2_NEW_CID means a catalog change.
+         */
+        if (SnapBuildCurrentState(builder) >= SNAPBUILD_BUILDING_SNAPSHOT &&
+            TransactionIdIsValid(xid) && info == XLOG_HEAP2_NEW_CID)
+            ReorderBufferXidSetCatalogChanges(ctx->reorder, xid, buf->origptr);
+
         return;
+    }
```

If we can commit the patch in [1] first, it can just be replaced by:

```
-    if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT)
+    if (SnapBuildCurrentState(builder) < SNAPBUILD_BUILDING_SNAPSHOT)
         return;
```

Thoughts?

[1]: https://www.postgresql.org/message-id/flat/tencent_3A071B760AA1A38540B57F297332B7781C08%40qq.com

--
Regards,
ChangAo Chen

On Sat, Nov 22, 2025 at 8:28 PM ocean_li_996 <ocean_li_996@163.com> wrote:
>
> Hi all,
>
> I would like to share a logical replication bug and some possible fixes. It seems that this bug has existed since
> logical replication was first introduced, so it has been around for quite some time. In fact, the previously
> reported issues [1], [2], [3] were all caused by this bug.
>
> # Problem description
>
> When in the BUILDING_SNAPSHOT state, the snapshot builder does not track the status of any
> transaction. It can lead to missing transaction states when:
> -- The transaction commits before the builder reaches FULL_SNAPSHOT state, and
> -- The transaction's xid is greater than or equal to builder->xmin when the builder reaches
> FULL_SNAPSHOT state.

> 2) Based on v6-0001, I have provided a minimal fix in v6-0003 (not yet reviewed). AFAICS, it resolves
> the problem, though it records additional useless information in the reorder buffer during BUILDING_SNAPSHOT
> state (which is discarded later). This increases memory usage and slightly impacts performance. But since
> snapshot building is infrequent, I consider this acceptable.
>
> 3) I have also prepared a cleaner and more efficient fix in v6-0004 than v6-0003, albeit more complex
> (similar to v6-0001). Provided as an alternative reference.

Hello Haiyang,

I agree with your analysis and approach, but when I tried out the
patch (applying patch 0002 for the tests and patch 0004), I see the
tests in contrib/test_decoding failing.
Similarly, applying patch 0002 and 0003 also results in the tests
failing. So, I am not sure how your minimal fix fixes the problem. Am
I doing something wrong?
Does patch 0003 and 0004 have to be applied on top of 0001? That
doesn't seem to be the case, as both make the same code change and
don't apply cleanly.

regards,
Ajin Cherian
Fujitsu Australia

On Thu, Jan 29, 2026 at 6:07 AM ocean_li_996 <ocean_li_996@163.com> wrote:
>
> Hi Ajin,
>
> At 2026-01-28 11:32:41, "Ajin Cherian" <itsajin@gmail.com> wrote:
> >I agree with your analysis and approach, but when I tried out the
> >patch (applying patch 0002 for the tests and patch 0004), I see the
> >tests in contrib/test_decoding failing.
> >Similarly, applying patch 0002 and 0003 also results in the tests
> >failing. So, I am not sure how your minimal fix fixes the problem. Am
> >I doing something wrong?
> >Does patch 0003 and 0004 have to be applied on top of 0001? That
> >doesn't seem to be the case, as both make the same code change and
> >don't apply cleanly.
>
> 0002 patch is only a test case. And 0001, 0003 and 0004 are independt fix patch.
>
> I appied 0002 + 0003 and 0002 + 0004 separately in master. And both the tests in
>
> contrib/test_decoding were passed. Can you provide more details about the failed
>
> tests(such as which tests and the diff between expected and ressult).

Hi Haiyang,

I tested with patch 0002+0004 on HEAD, and the test added by patch
0002 is failing like below.

not ok 15    - snapshot_build                            289 ms
# (test process exited with exit code 1)

I see the postgres crashed and when I look at the core file, I see the
below stack trace:
Core was generated by
`/home/ajin/postgresql/postgres3/postgres/tmp_install/home/ajin/install-oss/bin/postgres
'' '' '' '' '' '' '''.
Program terminated with signal SIGABRT, Aborted.
#0  0x00007436092c7e9c in __pthread_kill_implementation () from /lib64/libc.so.6
Missing rpms, try: dnf --enablerepo='*debug*' install
zlib-ng-compat-debuginfo-2.2.3-2.el10.x86_64
glibc-debuginfo-2.39-38.el10.x86_64
libicu-debuginfo-74.2-5.el10.x86_64
libstdc++-debuginfo-14.3.1-2.1.el10.x86_64
libgcc-debuginfo-14.3.1-2.1.el10.x86_64
(gdb) bt
#0  0x00007436092c7e9c in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x0000743609271a96 in raise () from /lib64/libc.so.6
#2  0x00007436092598fa in abort () from /lib64/libc.so.6
#3  0x0000000000a00a93 in ExceptionalCondition
(conditionName=conditionName@entry=0xb1e039 "txn->ninvalidations ==
0", fileName=fileName@entry=0xb1dcd8 "reorderbuffer.c",
lineNumber=lineNumber@entry=3207) at assert.c:65
#4  0x0000000000836c94 in ReorderBufferForget (rb=0x3cf96a40,
xid=xid@entry=1136, lsn=34819568) at reorderbuffer.c:3207
#5  0x0000000000823aea in DecodeCommit (ctx=ctx@entry=0x3cf869d0,
buf=buf@entry=0x7ffd069389a0, parsed=parsed@entry=0x7ffd069387d0,
xid=xid@entry=1136, two_phase=false) at decode.c:707
#6  0x000000000082497c in xact_decode (ctx=ctx@entry=0x3cf869d0,
buf=buf@entry=0x7ffd069389a0) at decode.c:237
#7  0x00000000008246eb in LogicalDecodingProcessRecord
(ctx=ctx@entry=0x3cf869d0, record=0x3cf86da8) at decode.c:116
#8  0x0000000000829602 in DecodingContextFindStartpoint
(ctx=ctx@entry=0x3cf869d0) at logical.c:647
#9  0x000000000084e8ea in create_logical_replication_slot
(name=name@entry=0x3ce8fb70 "isolation_slot",
plugin=plugin@entry=0x3ce8fc10 "test_decoding",
temporary=temporary@entry=false, two_phase=two_phase@entry=false,
failover=failover@entry=false,
    restart_lsn=restart_lsn@entry=0, find_startpoint=true) at slotfuncs.c:177
#10 0x000000000084f300 in pg_create_logical_replication_slot
(fcinfo=<optimized out>) at slotfuncs.c:207
#11 0x00000000006c77c4 in ExecMakeTableFunctionResult
(setexpr=0x3cf61b18, econtext=0x3cf61968, argContext=<optimized out>,
expectedDesc=0x3cf79f48, randomAccess=false) at execSRF.c:234
#12 0x00000000006da839 in FunctionNext (node=node@entry=0x3cf61758) at
nodeFunctionscan.c:94

(gdb) f 4
#4  0x0000000000836c94 in ReorderBufferForget (rb=0x3cf96a40,
xid=xid@entry=1136, lsn=34819568) at reorderbuffer.c:3207
3207                    Assert(txn->ninvalidations == 0);
(gdb) p *txn
$1 = {txn_flags = 1, xid = 1136, toplevel_xid = 0, gid = 0x0,
first_lsn = 34814328, final_lsn = 34819568, end_lsn = 0, toptxn = 0x0,
restart_decoding_lsn = 0, origin_id = 0, origin_lsn = 0, {commit_time
= 0, prepare_time = 0, abort_time = 0}, base_snapshot = 0x0,
  base_snapshot_lsn = 0, base_snapshot_node = {prev = 0x0, next =
0x0}, snapshot_now = 0x0, command_id = 4294967295, nentries = 14,
nentries_mem = 14, changes = {head = {prev = 0x3cfb5510, next =
0x3cfb4ae8}}, tuplecids = {head = {prev = 0x3cfb5440, next =
0x3cfb4a80}},
  ntuplecids = 13, tuplecid_hash = 0x0, toast_hash = 0x0, subtxns =
{head = {prev = 0x3cfb2c68, next = 0x3cfb2c68}}, nsubtxns = 0,
ninvalidations = 22, invalidations = 0x3cf96c50,
ninvalidations_distributed = 0, invalidations_distributed = 0x0, node
= {prev = 0x3cfb2b30,
    next = 0x3cf96a48}, catchange_node = {prev = 0x3cf96a68, next =
0x3cf96a68}, txn_node = {first_child = 0x0, next_sibling = 0x0,
prev_or_parent = 0x0}, size = 1472, total_size = 1472,
output_plugin_private = 0x0}

Looks like the assert in ReorderBufferForget failed because
ninvalidations is not 0.

You need to build with asserts enabled.

regards,
Ajin Cherian
Fujtitsu Australia

> Looks like the assert in ReorderBufferForget failed because
> ninvalidations is not 0.

I think it can be fixed by this:

```
@@ -282,18 +286,24 @@ xact_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
             {
                 TransactionId xid;
                 xl_xact_invals *invals;
+                bool has_snapshot;
 
                 xid = XLogRecGetXid(r);
                 invals = (xl_xact_invals *) XLogRecGetData(r);
+                has_snapshot =
+                    SnapBuildCurrentState(builder) >= SNAPBUILD_FULL_SNAPSHOT;
 
                 /*
                  * Execute the invalidations for xid-less transactions,
                  * otherwise, accumulate them so that they can be processed at
                  * the commit time.
+                 *
+                 * Note that we only need to do this when we are not fast-forwarding
+                 * and there is a snapshot.
                  */
                 if (TransactionIdIsValid(xid))
                 {
-                    if (!ctx->fast_forward)
+                    if (!ctx->fast_forward && has_snapshot)
                         ReorderBufferAddInvalidations(reorder, xid,
                                                       buf->origptr,
                                                       invals->nmsgs,
@@ -301,7 +311,7 @@ xact_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
                     ReorderBufferXidSetCatalogChanges(ctx->reorder, xid,
                                                       buf->origptr);
                 }
-                else if (!ctx->fast_forward)
+                else if (!ctx->fast_forward && has_snapshot)
                     ReorderBufferImmediateInvalidation(ctx->reorder,
                                                        invals->nmsgs,
                                                        invals->msgs);
```

--
Regards,
ChangAo Chen

On Thu, Jan 29, 2026 at 5:29 PM cca5507 <cca5507@qq.com> wrote:
>
> > Looks like the assert in ReorderBufferForget failed because
> > ninvalidations is not 0.
>
> I think it can be fixed by this:
>
> ```
> @@ -282,18 +286,24 @@ xact_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
>                         {
>                                 TransactionId xid;
>                                 xl_xact_invals *invals;
> +                               bool has_snapshot;
>
>                                 xid = XLogRecGetXid(r);
>                                 invals = (xl_xact_invals *) XLogRecGetData(r);
> +                               has_snapshot =
> +                                       SnapBuildCurrentState(builder) >= SNAPBUILD_FULL_SNAPSHOT;
>
>                                 /*
>                                  * Execute the invalidations for xid-less transactions,
>                                  * otherwise, accumulate them so that they can be processed at
>                                  * the commit time.
> +                                *
> +                                * Note that we only need to do this when we are not fast-forwarding
> +                                * and there is a snapshot.
>                                  */
>                                 if (TransactionIdIsValid(xid))
>                                 {
> -                                       if (!ctx->fast_forward)
> +                                       if (!ctx->fast_forward && has_snapshot)
>                                                 ReorderBufferAddInvalidations(reorder, xid,
>
buf->origptr,
>
invals->nmsgs,
> @@ -301,7 +311,7 @@ xact_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
>                                         ReorderBufferXidSetCatalogChanges(ctx->reorder, xid,
>
buf->origptr);
>                                 }
> -                               else if (!ctx->fast_forward)
> +                               else if (!ctx->fast_forward && has_snapshot)
>                                         ReorderBufferImmediateInvalidation(ctx->reorder,
>
invals->nmsgs,
>
invals->msgs);
> ```
>
> --

Yes, this works.

regards,
Ajin Cherian
Fujitsu Australia