On Tue, May 27, 2025 at 3:59 PM shveta malik <shveta.ma...@gmail.com> wrote:
>
> On Mon, May 26, 2025 at 12:46 PM Zhijie Hou (Fujitsu)
> <houzj.f...@fujitsu.com> wrote:
> >
> > Attaching the V32 patch set which addressed comments in [1]~[5].
>
> Thanks for the patch, I am still reviewing the patches, please find
> few trivial comments for patch001:
>
> 1)
>
> + FullTransactionId last_phase_at; /* publisher transaction ID that must
> + * be awaited to complete before
> + * entering the final phase
> + * (RCI_WAIT_FOR_LOCAL_FLUSH) */
>
> 'last_phase_at' seems like we are talking about the phase in the past.
> (similar to 'last' in last_recv_time).
> Perhaps we should name it as 'final_phase_at'
>
I am not sure the phase in this variable name matches with what it is
used for. The other option could be remote_wait_for or something on
those lines.
Additionally, please find a few cosmetic changes atop 0001 and 0002
patches. Please include in next set, if those looks okay to you.
--
With Regards,
Amit Kapila.
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
