v30_0001_amit.1.patch.txt

text/plain

Filename: v30_0001_amit.1.patch.txt
Type: text/plain
Part: 0
Message: Re: Conflict detection for update_deleted in logical replication
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)