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

From: "cca5507" <[email-masked]>
Date: Mon, 12 Aug 2024 16:34:25 +0800

Hi,

4, 5 ===

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

I think during fast forward, we also need handle the xlog that marks a transaction
as&nbsp;catalog modifying, or the snapshot might lose some transactions?

&gt; That way we'd still rely on what's being done in the XLOG_HEAP_INPLACE case

+		if (SnapBuildCurrentState(builder) &gt;= SNAPBUILD_BUILDING_SNAPSHOT)
+		{
+			/* Currently only XLOG_HEAP_INPLACE means a catalog modifying */
+			if (info == XLOG_HEAP_INPLACE &amp;&amp; TransactionIdIsValid(xid))
+				ReorderBufferXidSetCatalogChanges(ctx-&gt;reorder, xid, buf-&gt;origptr);
+		}

We only call&nbsp;ReorderBufferXidSetCatalogChanges() for the xlog that marks a transaction as&nbsp;catalog
modifying, and we don't care about the other steps being done in the xlog, so I think the current
approach is ok.

--
Regards,
ChangAo Chen

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 12 Aug 2024 10:35:44 +0000

Hi,

> as&nbsp;catalog modifying, or the snapshot might lose some transactions?

I think it's fine to skip during fast forward as we are not generating logical
changes. It's done that way in master, in your proposal and in my "if" proposals.
Note that my proposals related to the if conditions are for heap2_decode and
heap_decode (not xact_decode).

> modifying, and we don't care about the other steps being done in the xlog, so I think the current

Yeah, I think your proposal does not do anything wrong. I just prefer to put
everything in a single if condition (as per my proposal) so that we can jump
directly in the appropriate case. I think that way the code is easier to maintain
instead of having to deal with the same info values in distinct places.

Regards,

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

---

From: "cca5507" <[email-masked]>
Date: Mon, 12 Aug 2024 19:38:53 +0800

Hi,

Thanks for the comments!

- I think it's fine to skip during fast forward as we are not generating logical
- changes. It's done that way in master, in your proposal and in my "if" proposals.
- Note that my proposals related to the if conditions are for heap2_decode and
- heap_decode (not xact_decode).

But note that in xact_decode(), case&nbsp;XLOG_XACT_INVALIDATIONS, we call
ReorderBufferXidSetCatalogChanges() even if we are fast-forwarding, it might
be better to be consistent.

In addition, we don't decode anything during fast forward, but the snapshot might
serialize to disk. If we skip calling ReorderBufferXidSetCatalogChanges(), the snapshot
may be wrong on disk.

--
Regards,
ChangAo Chen

---

From: "cca5507" <[email-masked]>
Date: Tue, 13 Aug 2024 12:23:04 +0800

Hi,

I refactor the code and fix the git apply warning&nbsp;according to [1].

Here are the new version patches.

--
Regards,
ChangAo Chen

[1]&nbsp;https://www.postgresql.org/message-id/Zrmh7X8jYCbFYXjH%40ip-10-97-1-34.eu-west-3.compute.internal

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 13 Aug 2024 06:19:54 +0000

Hi,

> 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,

---

From: Ajin Cherian <[email-masked]>
Date: Fri, 27 Jun 2025 20:29:53 +1000

On Fri, Jun 27, 2025 at 3:48 PM cca5507 <[email-masked]> wrote:
>
> Hi,
>
> I refactor the code and fix the git apply warning according to [1].
>
> [1] https://www.postgresql.org/message-id/Zrmh7X8jYCbFYXjH%40ip-10-97-1-34.eu-west-3.compute.internal

I see this problem is similar to the bug reported in [1], and your fix
also addresses the issue reported there. Although I like your approach
of tracking changes starting from the BUILDING_SNAPSHOT state, I’d
like to suggest an alternative.

While debugging that issue, my plan was not to track catalog changes
prior to SNAPBUILD_CONSISTENT, but instead to ensure we don’t use
snapshots built before SNAPBUILD_CONSISTENT, since we don’t track
catalog changes in those states. We should discard previously built
snapshots and rebuild them once we reach the SNAPBUILD_CONSISTENT
state. At that point, all necessary transactions would have been
committed, and builder->xmin would have advanced enough to decode all
transactions from then on.

The problem is that previously built snapshots hang around without the
latest xmin and xmax, and we tend to reuse them. We should ensure that
all txn->base_snapshot and builder->snapshot snapshots built in the
SNAPBUILD_FULL_SNAPSHOT state are rebuilt once we reach
SNAPBUILD_CONSISTENT. For this, we need to track when the snapshot was
built. There is already a field in ReorderBufferTXN -
'base_snapshot_lsn' which we can use. If base_snapshot_lsn <
builder->start_decoding_at, then we should rebuild the snapshot. Just
a thought.

regards,
Ajin Cherian
Fujitsu Australia

[1] - https://www.postgresql.org/message-id/18509-983f064d174ea880%40postgresql.org

---

From: "cca5507" <[email-masked]>
Date: Sat, 28 Jun 2025 11:44:52 +0800

Hi,

If I understand correctly, this may break the basic&nbsp;principle:

The aim is to build a&nbsp;snapshot that behaves the same as a freshly taken MVCC snapshot would have&nbsp;at the time the XLogRecord was generated.

--
Regards,
ChangAo Chen

------------------&nbsp;Original&nbsp;------------------
From:                                                                                                                        "Ajin Cherian"                                                                                    <[email-masked]&gt;;
Date:&nbsp;Fri, Jun 27, 2025 06:29 PM
To:&nbsp;"cca5507"<[email-masked]&gt;;
Cc:&nbsp;"Bertrand Drouvot"<[email-masked]&gt;;"Michael Paquier"<[email-masked]&gt;;"pgsql-hackers"<[email-masked]&gt;;
Subject:&nbsp;Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state

On Fri, Jun 27, 2025 at 3:48 PM cca5507 <[email-masked]&gt; wrote:
&gt;
&gt; Hi,
&gt;
&gt; I refactor the code and fix the git apply warning according to [1].
&gt;
&gt; Here are the new version patches.
&gt;
&gt; --
&gt; Regards,
&gt; ChangAo Chen
&gt;
&gt; [1] https://www.postgresql.org/message-id/Zrmh7X8jYCbFYXjH%40ip-10-97-1-34.eu-west-3.compute.internal

committed, and builder-&gt;xmin would have advanced enough to decode all

The problem is that previously built snapshots hang around without the
latest xmin and xmax, and we tend to reuse them. We should ensure that
all txn-&gt;base_snapshot and builder-&gt;snapshot snapshots built in the
SNAPBUILD_FULL_SNAPSHOT state are rebuilt once we reach
SNAPBUILD_CONSISTENT. For this, we need to track when the snapshot was
built. There is already a field in ReorderBufferTXN -
'base_snapshot_lsn' which we can use. If base_snapshot_lsn <
builder-&gt;start_decoding_at, then we should rebuild the snapshot. Just
a thought.

regards,
Ajin Cherian
Fujitsu Australia

---

From: Michael Paquier <[email-masked]>
Date: Wed, 2 Jul 2025 08:47:03 +0900

On Sat, Jun 28, 2025 at 11:44:52AM +0800, cca5507 wrote:
> The aim is to build a&nbsp;snapshot that behaves the same as a
> freshly taken MVCC snapshot would have&nbsp;at the time the
> XLogRecord was generated. 

Please note that we prefer bottom-posting when sending messages on
pgsql-hackers and the community mailing lists, as defined in this
link:
https://en.wikipedia.org/wiki/Posting_style#Bottom-posting

Top-posting, as you did in your last email, is breaking the logic and
flow of the discussion.

Some more useful documentation from the PostgreSQL wiki:
https://wiki.postgresql.org/wiki/Mailing_Lists
--
Michael

---

From: "cca5507" <[email-masked]>
Date: Fri, 21 Nov 2025 16:16:16 +0800

Hi all,

Attach the rebased patches.

Some useful link:

Discussion:
https://www.postgresql.org/message-id/flat/[email-masked]
https://www.postgresql.org/message-id/flat/[email-masked]
https://www.postgresql.org/message-id/flat/[email-masked]

Bug Report:
https://www.postgresql.org/message-id/flat/18509-983f064d174ea880%40postgresql.org
https://www.postgresql.org/message-id/flat/19109-4a9ce36715d69632%40postgresql.org

CF Entry:
https://commitfest.postgresql.org/patch/5029/

--
Regards,
ChangAo Chen