On Mon, Mar 17, 2025 at 6:53 AM Hayato Kuroda (Fujitsu)
<[email protected]> wrote:
>
> OK, let me share patched for back branches. Mostly the same fix patched as
> master
> can be used for PG14-PG17, like attached.
>
A few comments:
==============
1.
+SnapBuildDistributeSnapshotAndInval(SnapBuild *builder, XLogRecPtr
lsn, TransactionId xid)
{
dlist_iter txn_i;
ReorderBufferTXN *txn;
+ ReorderBufferTXN *curr_txn;
+
+ curr_txn = ReorderBufferTXNByXid(builder->reorder, xid, false, NULL,
InvalidXLogRecPtr, false);
The above is used to access invalidations from curr_txn. I am thinking
about whether it would be better to expose a new function to get
invalidations for a txn based on xid instead of getting
ReorderBufferTXN. It would avoid any layering violation and misuse of
ReorderBufferTXN by other modules.
2. The patch has a lot of tests to verify the same points. Can't we
have one simple test using SQL API based on what Andres presented in
an email [1]?
3. I have made minor changes in the comments in the attached.
[1] -
https://www.postgresql.org/message-id/20231119021830.d6t6aaxtrkpn743y%40awork3.anarazel.de
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/logical/snapbuild.c
b/src/backend/replication/logical/snapbuild.c
index 16acb506141..fd1a3e75b29 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -739,7 +739,8 @@ SnapBuildDistributeSnapshotAndInval(SnapBuild *builder,
XLogRecPtr lsn, Transact
/*
* Iterate through all toplevel transactions. This can include
* subtransactions which we just don't yet know to be that, but that's
- * fine, they will just get an unnecessary snapshot queued.
+ * fine, they will just get an unnecessary snapshot and invalidations
+ * queued.
*/
dlist_foreach(txn_i, &builder->reorder->toplevel_by_lsn)
{
@@ -787,12 +788,12 @@ SnapBuildDistributeSnapshotAndInval(SnapBuild *builder,
XLogRecPtr lsn, Transact
builder->snapshot);
/*
- * Add invalidation messages to the reorder buffer of inprogress
+ * Add invalidation messages to the reorder buffer of
in-progress
* transactions except the current committed transaction, for
which we
* will execute invalidations at the end.
*
* It is required, otherwise, we will end up using the stale
catcache
- * contents built by the current transaction even after its
decoding
+ * contents built by the current transaction even after its
decoding,
* which should have been invalidated due to concurrent catalog
* changing transaction.
*/
@@ -1071,8 +1072,8 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn,
TransactionId xid,
SnapBuildSnapIncRefcount(builder->snapshot);
/*
- * add a new catalog snapshot and invalidations messages to all
- * currently running transactions
+ * Add a new catalog snapshot and invalidations messages to all
+ * currently running transactions.
*/
SnapBuildDistributeSnapshotAndInval(builder, lsn, xid);
}
