From: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
---|---|
To: | cca5507 <cca5507(at)qq(dot)com> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state |
Date: | 2024-08-09 12:07:48 |
Message-ID: | ZrYGlKqKDBRWa4L8@ip-10-97-1-34.eu-west-3.compute.internal |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Marcos Pegoraro | 2024-08-09 12:42:53 | Re: Detailed release notes |
Previous Message | Michail Nikolaev | 2024-08-09 11:45:12 | Re: Conflict detection and logging in logical replication |