v32_amit_1.patch.txt

text/plain

Filename: v32_amit_1.patch.txt
Type: text/plain
Part: 0
Message: Re: Conflict detection for update_deleted in logical replication
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 740ec89e070..fe558f0a81c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4961,8 +4961,8 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
           new setting.
           This setting has no effect if <varname>primary_conninfo</varname> is not
           set or the server is not in standby mode.
-          The name cannot be <literal>pg_conflict_detection</literal>, as it is
-          reserved for logical replication conflict detection.
+          The name cannot be <literal>pg_conflict_detection</literal> as it is
+          reserved for the conflict detection slot.
          </para>
         </listitem>
        </varlistentry>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4d98bf9bf19..5866dff9490 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -29711,8 +29711,8 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
        <para>
         Creates a new physical replication slot named
         <parameter>slot_name</parameter>. The name cannot be
-        <literal>pg_conflict_detection</literal>, as it is reserved for
-        logical replication conflict detection. The optional second parameter,
+        <literal>pg_conflict_detection</literal> as it is reserved for the
+        conflict detection slot. The optional second parameter,
         when <literal>true</literal>, specifies that the <acronym>LSN</acronym> for this
         replication slot be reserved immediately; otherwise
         the <acronym>LSN</acronym> is reserved on first connection from a streaming
@@ -29757,8 +29757,8 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
         Creates a new logical (decoding) replication slot named
         <parameter>slot_name</parameter> using the output plugin
         <parameter>plugin</parameter>. The name cannot be
-        <literal>pg_conflict_detection</literal>, as it is reserved for
-        logical replication conflict detection. The optional third
+        <literal>pg_conflict_detection</literal> as it is reserved for
+        the conflict detection slot. The optional third
         parameter, <parameter>temporary</parameter>, when set to true, specifies that
         the slot should not be permanently stored to disk and is only meant
         for use by the current session. Temporary slots are also
@@ -29789,7 +29789,7 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
         Copies an existing physical replication slot named <parameter>src_slot_name</parameter>
         to a physical replication slot named <parameter>dst_slot_name</parameter>.
         The new slot name cannot be <literal>pg_conflict_detection</literal>,
-        as it is reserved for logical replication conflict detection.
+        as it is reserved for the conflict detection.
         The copied physical slot starts to reserve WAL from the same <acronym>LSN</acronym> as the
         source slot.
         <parameter>temporary</parameter> is optional. If <parameter>temporary</parameter>
@@ -29813,9 +29813,9 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
         named <parameter>src_slot_name</parameter> to a logical replication
         slot named <parameter>dst_slot_name</parameter>, optionally changing
         the output plugin and persistence.  The name cannot be
-        <literal>pg_conflict_detection</literal>, as it is reserved for
-        logical replication conflict detection.  The copied logical slot starts
-        from the same <acronym>LSN</acronym> as the source logical slot.  Both
+        <literal>pg_conflict_detection</literal> as it is reserved for
+        the conflict detection.  The copied logical slot starts from the same
+        <acronym>LSN</acronym> as the source logical slot.  Both
         <parameter>temporary</parameter> and <parameter>plugin</parameter> are
         optional; if they are omitted, the values of the source slot are used.
         The <literal>failover</literal> option of the source logical slot
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 16d9171bf0d..cbd36641161 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2225,8 +2225,8 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
          <para>
           The name of the slot to create. Must be a valid replication slot
           name (see <xref linkend="streaming-replication-slots-manipulation"/>).
-          The name cannot be <literal>pg_conflict_detection</literal>, as it
-          is reserved for logical replication conflict detection.
+          The name cannot be <literal>pg_conflict_detection</literal> as it
+          is reserved for the conflict detection.
          </para>
         </listitem>
        </varlistentry>
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 739161df715..dba0f541ac5 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -170,8 +170,8 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
          <para>
           Name of the publisher's replication slot to use.  The default is
           to use the name of the subscription for the slot name. The name cannot
-          be <literal>pg_conflict_detection</literal>, as it is reserved for
-          logical replication conflict detection.
+          be <literal>pg_conflict_detection</literal> as it is reserved for the
+          conflict detection.
          </para>
 
          <para>
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 6a59db47583..17963486795 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -3862,8 +3862,8 @@ LogicalRepApplyLoop(XLogRecPtr last_received)
 			wait_time = NAPTIME_PER_CYCLE;
 
 		/*
-		 * Ensure to wake up when it's possible to attempt to advance the
-		 * non-removable transaction ID.
+		 * Ensure to wake up when it's possible to advance the non-removable
+		 * transaction ID.
 		 */
 		if (data.phase == RCI_GET_CANDIDATE_XID && data.xid_advance_interval)
 			wait_time = Min(wait_time, data.xid_advance_interval);
@@ -4103,11 +4103,13 @@ send_feedback(XLogRecPtr recvpos, bool force, bool requestReply)
  * WALs that are being replicated from the primary and those WALs could have
  * earlier commit timestamp.
  *
- * XXX It might seem feasible to track the latest commit timestamp on the
- * publisher and send the WAL position once the timestamp exceeds that on the
- * subscriber. However, commit timestamps can regress since a commit with a
- * later LSN is not guaranteed to have a later timestamp than those with
- * earlier LSNs.
+ * XXX It seems feasible to get the latest commit's WAL location from the
+ * publisher and wait till that is applied. However, we can't do that
+ * because commit timestamps can regress as a commit with a later LSN is not
+ * guaranteed to have a later timestamp than those with earlier LSNs. Having
+ * said that, even if that is possible, it won't improve performance much as
+ * the apply always lag and moves slowly as compared with the transactions
+ * on the publisher.
  */
 static void
 maybe_advance_nonremovable_xid(RetainConflictInfoData *rci_data,
@@ -4211,6 +4213,10 @@ get_candidate_xid(RetainConflictInfoData *rci_data)
 	 */
 	full_xid = FullTransactionIdFromAllowableAt(next_full_xid, oldest_running_xid);
 
+	/*
+	 * Oldest active transaction ID (full_xid) can't be behind any of its
+	 * previously computed value.
+	 */
 	Assert(FullTransactionIdPrecedesOrEquals(MyLogicalRepWorker->oldest_nonremovable_xid,
 											 full_xid));
 
@@ -4294,12 +4300,12 @@ wait_for_publisher_status(RetainConflictInfoData *rci_data,
 	 *
 	 * It's possible that transactions in the commit phase during the last
 	 * cycle have now finished committing, but remote_oldestxid remains older
-	 * than last_phase_at. This can happen if some old transaction was in the
+	 * than last_phase_at. This can happen if some old transaction came in the
 	 * commit phase when we requested status in this cycle. We do not handle
 	 * this case explicitly as it's rare and the benefit doesn't justify the
 	 * required complexity. Tracking would require either caching all xids at
 	 * the publisher or sending them to subscribers. The condition will
-	 * resolve naturally once the remaining transaction finishes.
+	 * resolve naturally once the remaining transactions are finished.
 	 *
 	 * Directly advancing the non-removable transaction ID is possible if
 	 * there are no activities on the publisher since the last advancement