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

Hi,

On Sat, Aug 10, 2024 at 06:07:30PM +0800, cca5507 wrote:
> Hi,
> 
> 
> Thanks for the comments!
> 
> 
> Here are the new version patches, I think it will be more clear.

Thanks!

1 ===

When applying I get:

Applying: Track transactions committed in BUILDING_SNAPSHOT.
.git/rebase-apply/patch:71: space before tab in indent.
                 */
.git/rebase-apply/patch:94: space before tab in indent.
                 */
warning: 2 lines add whitespace errors.

2 ===

+        * have snapshot and the transaction will not be tracked by snapshot

s/have snapshot/have a snapshot/?

3 ===

+        * snapshot and will not be decoded

s/snapshot/a snapshot/?


4 ===

        if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
                ctx->fast_forward)
+       {
+               /*
+                * Note that during or after BUILDING_SNAPSHOT, we need handle the xlog
+                * that might mark a transaction as catalog modifying because the snapshot
+                * only tracks catalog modifying transactions. The transaction before
+                * BUILDING_SNAPSHOT will not be tracked anyway(see SnapBuildCommitTxn()
+                * for details), so just return.
+                */
+               if (SnapBuildCurrentState(builder) >= SNAPBUILD_BUILDING_SNAPSHOT)
+               {
+                       /* Currently only XLOG_HEAP2_NEW_CID means a catalog modifying */
+                       if (info == XLOG_HEAP2_NEW_CID && TransactionIdIsValid(xid))

What about?

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

That way we'd still rely on what's being done in the XLOG_HEAP2_NEW_CID case (
should it change in the future).

5 ===

        if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
                ctx->fast_forward)
+       {
+               /*
+                * Note that during or after BUILDING_SNAPSHOT, we need handle the xlog
+                * that might mark a transaction as catalog modifying because the snapshot
+                * only tracks catalog modifying transactions. The transaction before
+                * BUILDING_SNAPSHOT will not be tracked anyway(see SnapBuildCommitTxn()
+                * for details), so just return.
+                */
+               if (SnapBuildCurrentState(builder) >= SNAPBUILD_BUILDING_SNAPSHOT)
+               {

What about?

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

That way we'd still rely on what's being done in the XLOG_HEAP_INPLACE case (
should it change in the future).

6 ===

v3-0002 LGTM.

Regards,

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