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

Hi,

On Tue, Aug 13, 2024 at 12:23:04PM +0800, cca5507 wrote:
> Hi,
> 
> I refactor the code and fix the git apply warning according to [1].
> 
> 
> Here are the new version patches.

Thanks!

1 ===

+       /* True if the xlog marks the transaction as containing catalog changes */
+       bool    set_catalog_changes = (info == XLOG_HEAP2_NEW_CID);

        if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
                ctx->fast_forward)
+       {
+               /*
+                * If the transaction contains catalog changes, we need mark it in
+                * reorder buffer before return as the snapshot only tracks catalog
+                * modifying transactions. The transaction before BUILDING_SNAPSHOT
+                * won't be tracked anyway(see SnapBuildCommitTxn), so skip it.
+                */
+               if (set_catalog_changes && TransactionIdIsValid(xid) &&
+                       SnapBuildCurrentState(builder) >= SNAPBUILD_BUILDING_SNAPSHOT)
+                       ReorderBufferXidSetCatalogChanges(ctx->reorder, xid, buf->origptr);
+
                return;
+       }

I still prefer to replace the above with:

if (SnapBuildCurrentState(builder) < SNAPBUILD_BUILDING_SNAPSHOT ||
    (SnapBuildCurrentState(builder) == SNAPBUILD_BUILDING_SNAPSHOT && info != XLOG_HEAP2_NEW_CID) ||
    ctx->fast_forward)
    return;

Let's see what others think.

2 ===

+       /* True if the xlog marks the transaction as containing catalog changes */
+       bool    set_catalog_changes = (info == XLOG_HEAP_INPLACE);

        if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
                ctx->fast_forward)
+       {
+               /*
+                * If the transaction contains catalog changes, we need mark it in
+                * reorder buffer before return as the snapshot only tracks catalog
+                * modifying transactions. The transaction before BUILDING_SNAPSHOT
+                * won't be tracked anyway(see SnapBuildCommitTxn), so skip it.
+                */
+               if (set_catalog_changes && TransactionIdIsValid(xid) &&
+                       SnapBuildCurrentState(builder) >= SNAPBUILD_BUILDING_SNAPSHOT)
+                       ReorderBufferXidSetCatalogChanges(ctx->reorder, xid, buf->origptr);
+
                return;
+       }

I still prefer to replace the above with:

if (SnapBuildCurrentState(builder) < SNAPBUILD_BUILDING_SNAPSHOT ||
    (SnapBuildCurrentState(builder) == SNAPBUILD_BUILDING_SNAPSHOT && info != XLOG_HEAP_INPLACE) ||
    ctx->fast_forward)
    return;

Let's see what others think.

3 ===

I re-read your comments in [0] and it looks like you've concern about
the 2 "if" I'm proposing above and the fast forward handling. Is that the case
or is your fast forward concern unrelated to my proposals?



Not sure what happened but it looks like your reply in [0] is not part of the
initial thread [1], but created a new thread instead, making the whole
conversation difficult to follow.

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

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com