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