Subject: Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state

From: Ajin Cherian <[email-masked]>
Date: Thu, 27 Mar 2025 13:30:32 +1100

On Thu, Mar 27, 2025 at 1:13 PM cca5507 <[email-masked]> wrote:
>
> Hi,
>
> - IIUC your "fast forward" concern is not related to this particular thread but you
> - think it's already an issue on the master branch (outside of the BUILDING_SNAPSHOT
> - handling we are discussing here), is that correct? (that's also what your coding
> - changes makes me think of). If so, I'd suggest to open a dedicated thread for that
> - particular "fast forward" point and do the coding in the current thread as if the
> - fast forward is not an issue.
>
> - Does that make sense?
>
> Yes.
>
> But I think the v4-0001 in [1] is fine.
>

@@ -1301,6 +1335,7 @@ DecodeTXNNeedSkip(LogicalDecodingContext *ctx,
XLogRecordBuffer *buf,
                   Oid txn_dbid, RepOriginId origin_id)
 {
     if (SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr) ||
+        SnapBuildCurrentState(ctx->snapshot_builder) < SNAPBUILD_CONSISTENT ||
         (txn_dbid != InvalidOid && txn_dbid != ctx->slot->data.database) ||
         FilterByOrigin(ctx, origin_id))
         return true;

I don't see an explanation as to why this change was added? Maybe I
missed it in the multiple threads. With this, we are no longer
decoding changes which were in the state SNAPBUILD_FULL_SNAPSHOT. Why
did that change?

regards,
Ajin Cherian
Fujitsu Australia

---

From: "cca5507" <[email-masked]>
Date: Thu, 27 Mar 2025 11:47:32 +0800

Hi,

According to the comment above DecodeTXNNeedSkip(), transactions committed before SNAPBUILD_CONSISTENT state won't be decoded. It's important for initial table data synchronization because the table sync workers use the consistent snapshot to copy data so transactions before SNAPBUILD_CONSISTENT are already included in the initial data.

This change may be redundant with SnapBuildXactNeedsSkip(), I add just for safe.

--
Regards,
ChangAo Chen

---

From: Ajin Cherian <[email-masked]>
Date: Thu, 27 Mar 2025 15:57:58 +1100

> This change may be redundant with SnapBuildXactNeedsSkip(), I add just for safe.

I understand your explanation but was there a bug on HEAD that this is
solving? If so, can you write a test case to show this? This will make
it more convincing that this change as well is required. Your other
change has a test case to confirm, but this doesn't.

regards,
Ajin Cherian
Fujitsu Australia

---

From: "cca5507" <[email-masked]>
Date: Thu, 27 Mar 2025 15:26:51 +0800

Hi,

This change doesn't solve a bug. But I think it makes the code and comments more consistent. I currently have no idea about how to test it.

--
Regards,
ChangAo Chen