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

Hi,

On Thu, Aug 08, 2024 at 03:53:29PM +0800, cca5507 wrote:
> Hi,
> 
> 
> Thanks for pointing it out!
> 
> 
> Here are the new version patches with a test case.

Thanks!

I think the approach that the patch implements makes sense and that we should
track the transactions that have been commmitted while building the snapshot.

A few random comments:

1 ===

+        * that the xlog in BUILDING_SNAPSHOT is only useful for build

s/for build/to build? (same comment for the commit message)

2 ===

+        * snapshot and will not be decoded.

worth to mention DecodeTXNNeedSkip() here?

3 ===

+        * point in decoding changes. Note that we only handle XLOG_HEAP2_NEW_CID
+        * which mark a transaction as catalog modifying in BUILDING_SNAPSHOT

do you mean? Note that during BUILDING_SNAPSHOT, we only handle XLOG_HEAP2_NEW_CID
as it marks a transaction as catalog modifying. If so, what about making the 

-       if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
+       if (SnapBuildCurrentState(builder) < SNAPBUILD_BUILDING_SNAPSHOT ||

more clear about that? (to avoid any work when it's not needed)

4 ===

+        * useful for build the snapshot.

s/for/to/?

5 ===

+        * point in decoding changes. Note that we only handle XLOG_HEAP_INPLACE
+        * which mark a transaction as catalog modifying in BUILDING_SNAPSHOT, it's

s/which mark/which might mark/?

do you mean? Note that during BUILDING_SNAPSHOT, we only handle XLOG_HEAP_INPLACE
as it might mark a transaction as catalog modifying. If so, what about making the 

-       if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
+       if (SnapBuildCurrentState(builder) < SNAPBUILD_BUILDING_SNAPSHOT ||

more clear about that? (to avoid any work when it's not needed)

Idea of 3 === and 5 === is to proceed further in the SNAPBUILD_BUILDING_SNAPSHOT
case only if we know that the transaction is a catalog changing one (or might
be one).

6 ===

+        * useful for build the snapshot.

s/for/to/?

7 ===

+# Test snapshot build correctly
+

what about? Test tracking of committed transactions during BUILDING_SNAPSHOT

Regards,

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