On Fri, May 16, 2025 at 5:01 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
Please find some more comments on the 0001 patch:
1.
We need to know about such transactions
+ * for conflict detection and resolution in logical replication. See
+ * GetOldestTransactionIdInCommit and its use.
Do we need to mention resolution in the above sentence? This patch is
all about detecting conflict reliably.
2. In wait_for_publisher_status(), we use remote_epoch,
remote_nextxid, and remote_oldestxid to compute full transaction id's.
Why can't we send FullTransactionIds for remote_oldestxid and
remote_nextxid from publisher? If these are required, maybe a comment
somewhere for that would be good.
3.
/*
+ * Note it is important to set committs value after marking ourselves as
+ * in the commit critical section (DELAY_CHKPT_IN_COMMIT). This is because
+ * we want to ensure all such transactions are finished before we allow
+ * the logical replication client to advance its xid which is used to hold
+ * back dead rows for conflict detection. See
+ * maybe_advance_nonremovable_xid.
+ */
+ committs = GetCurrentTimestamp();
How does setting committs after setting DELAY_CHKPT_IN_COMMIT help in
advancing client-side xid? IIUC, on client-side, we simply wait for
such an xid to be finished based on the remote_oldestxid and
remote_nextxid sent via the server. So, the above comment is not
completely clear to me. I have updated this and related comments in
the attached diff patch to make it clear. See if that makes sense to
you.
4.
In 0001's commit message, we have: "Furthermore, the preserved commit
timestamps and origin data are essential for
consistently detecting update_origin_differs conflicts." But it is not
clarified how this patch helps in consistently detecting
update_origin_differs conflict?
5. I have tried to add some more details in comments on why
oldest_nonremovable_xid needs to be FullTransactionId. See attached.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/access/transam/twophase.c
b/src/backend/access/transam/twophase.c
index 41fb4fc5025..ea508542745 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -2329,9 +2329,9 @@ RecordTransactionCommitPrepared(TransactionId xid,
/*
* Note it is important to set committs value after marking ourselves as
* in the commit critical section (DELAY_CHKPT_IN_COMMIT). This is
because
- * we want to ensure all such transactions are finished before we allow
- * the logical replication client to advance its xid which is used to
hold
- * back dead rows for conflict detection. See
+ * we want to ensure all transactions that have acquired commit
timestamp
+ * are finished before we allow the logical replication client to
advance
+ * its xid which is used to hold back dead rows for conflict detection.
See
* maybe_advance_nonremovable_xid.
*/
committs = GetCurrentTimestamp();
diff --git a/src/backend/access/transam/xact.c
b/src/backend/access/transam/xact.c
index dfc5108da3c..6e1dc76744f 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1431,6 +1431,10 @@ RecordTransactionCommit(void)
* without holding the ProcArrayLock, since we're the only one
* modifying it. This makes checkpoint's determination of
which xacts
* are delaying the checkpoint a bit fuzzy, but it doesn't
matter.
+ *
+ * Note, it is important to get the commit timestamp after
marking the
+ * transaction in the commit critical section. See
+ * RecordTransactionCommitPrepared.
*/
Assert((MyProc->delayChkptFlags & DELAY_CHKPT_IN_COMMIT) == 0);
START_CRIT_SECTION();
diff --git a/src/include/replication/worker_internal.h
b/src/include/replication/worker_internal.h
index 0fdf49a1938..7d1e2096232 100644
--- a/src/include/replication/worker_internal.h
+++ b/src/include/replication/worker_internal.h
@@ -99,7 +99,9 @@ typedef struct LogicalRepWorker
* conditions. Such scenarios might occur if the replication slot is not
* yet created by the launcher while the apply worker has already
* initialized this field. During this period, a transaction ID
wraparound
- * could falsely make this ID appear as if it originates from the
future.
+ * could falsely make this ID appear as if it originates from the future
+ * w.r.t the transaction ID stored in the slot maintained by launcher.
See
+ * advance_conflict_slot_xmin.
*/
FullTransactionId oldest_nonremovable_xid;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 65ecf3280fb..c6f5ebceefd 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -133,10 +133,10 @@ extern PGDLLIMPORT int FastPathLockGroupsPerBackend;
*
* Setting DELAY_CHKPT_IN_COMMIT is similar to setting DELAY_CHKPT_START, but
* it explicitly indicates that the reason for delaying the checkpoint is due
- * to a transaction being within a critical commit section. We need this to
- * ensure all such transactions are finished before we allow the logical
- * replication client to advance its xid which is used to hold back dead rows
- * for conflict detection.
+ * to a transaction being within a critical commit section. We need this new
+ * flag to ensure all the transactions that have acquired commit timestamp are
+ * finished before we allow the logical replication client to advance its xid
+ * which is used to hold back dead rows for conflict detection.
*/
#define DELAY_CHKPT_START (1<<0)
#define DELAY_CHKPT_COMPLETE (1<<1)
