On Thu, May 22, 2025 at 8:28 AM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> Attaching the V31 patch set which addressed comments in [1]~[8].
>
Few comments:
1.
<para>
+ The oldest transaction ID along that is currently in the commit
+ phase on the server, along with its epoch.
The first 'along' in the sentence looks redundant. I've removed this
in the attached.
2.
+ data.remote_oldestxid = FullTransactionIdFromU64(pq_getmsgint64(&s));
+ data.remote_nextxid = FullTransactionIdFromU64(pq_getmsgint64(&s));
Shouldn't we need to typecast the result of pq_getmsgint64(&s) with
uint64 as we do at similar other places in pg_snapshot_recv?
3.
+ pq_sendint64(&output_message,
U64FromFullTransactionId(fullOldestXidInCommit));
+ pq_sendint64(&output_message, U64FromFullTransactionId(nextFullXid));
Similarly, here also we should typecase with uint64
4.
+ * XXX In phase RCI_REQUEST_PUBLISHER_STATUS, a potential enhancement could be
+ * requesting transaction information specifically for those containing
+ * UPDATEs. However, this approach introduces additional complexities in
+ * tracking UPDATEs for transactions on the publisher, and it may not
+ * effectively address scenarios with frequent UPDATEs.
I think, as the patch needs the oldest_nonremovable_xid idea even to
detect update_origin_differs and delete_origin_differs reliably, as
mentioned in 0001's commit message, is it sufficient to track update
transactions? Don't we need to track it even for deletes? I have
removed this note for now and updated the comment to mention it is
required to detect update_origin_differs and delete_origin_differs
conflicts reliably.
Apart from the above comments, I made a few other cosmetic changes in
the attached.
--
With Regards,
Amit Kapila.
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 7a40c62c59b..16d9171bf0d 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2672,11 +2672,11 @@ psql "dbname=postgres replication=database" -c
"IDENTIFY_SYSTEM;"
<term>Int64</term>
<listitem>
<para>
- The oldest transaction ID along that is currently in the commit
- phase on the server, along with its epoch. The most significant 32
- bits are the epoch. The least significant 32 bits are the
- transaction ID. If no transactions are active on the server, this
- number will be the next transaction ID to be assigned.
+ The oldest transaction ID that is currently in the commit phase on
+ the server, along with its epoch. The most significant 32 bits are
+ the epoch. The least significant 32 bits are the transaction ID.
+ If no transactions are active on the server, this number will be
+ the next transaction ID to be assigned.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/replication/logical/worker.c
b/src/backend/replication/logical/worker.c
index e9ca6c88e7e..6b9477bdf6a 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -309,17 +309,16 @@ typedef struct RetainConflictInfoData
* (RCI_WAIT_FOR_LOCAL_FLUSH) */
FullTransactionId candidate_xid; /* candidate for the
non-removable
* transaction ID */
+ TimestampTz flushpos_update_time; /* when the remote flush
position was
+
* updated in final phase
+
* (RCI_WAIT_FOR_LOCAL_FLUSH) */
/*
* The following fields are used to determine the timing for the next
round
- * of transaction ID advancement and the update of the remote flush
- * position.
+ * of transaction ID advancement.
*/
TimestampTz last_recv_time; /* when the last message was received */
TimestampTz candidate_xid_time; /* when the candidate_xid is decided */
- TimestampTz flushpos_update_time; /* when the remote flush
position was
-
* updated in final phase
-
* (RCI_WAIT_FOR_LOCAL_FLUSH) */
int xid_advance_interval; /* how much time (ms)
to wait before
* attempting to advance the
* non-removable transaction ID */
@@ -4051,7 +4050,10 @@ send_feedback(XLogRecPtr recvpos, bool force, bool
requestReply)
*
* The oldest_nonremovable_xid is maintained in shared memory to prevent dead
* rows from being removed prematurely when the apply worker still needs them
- * to detect update_deleted conflicts.
+ * to detect update_deleted conflicts. Additionally, this helps to retain
+ * the required commit_ts module information, which further helps to detect
+ * update_origin_differs and delete_origin_differs conflicts reliably, as
+ * otherwise, vacuum freeze could remove the required information.
*
* The non-removable transaction ID is advanced to the oldest running
* transaction ID once all concurrent transactions on the publisher have been
@@ -4106,12 +4108,6 @@ send_feedback(XLogRecPtr recvpos, bool force, bool
requestReply)
* 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 In phase RCI_REQUEST_PUBLISHER_STATUS, a potential enhancement could be
- * requesting transaction information specifically for those containing
- * UPDATEs. However, this approach introduces additional complexities in
- * tracking UPDATEs for transactions on the publisher, and it may not
- * effectively address scenarios with frequent UPDATEs.
*/
static void
maybe_advance_nonremovable_xid(RetainConflictInfoData *data,
@@ -4201,8 +4197,7 @@ get_candidate_xid(RetainConflictInfoData *data)
/*
* Immediately update the timer, even if the function returns later
without
- * setting candidate_xid due to inactivity on the subscriber. This
ensures
- * a certain interval before recalculating candidate_xid, minimizing
+ * setting candidate_xid due to inactivity on the subscriber. This
avoids
* frequent calls to GetOldestActiveTransactionId.
*/
data->candidate_xid_time = now;
@@ -4298,8 +4293,8 @@ wait_for_publisher_status(RetainConflictInfoData *data,
bool status_received)
* However, it requires maintaining two fields, last_remote_nextxid and
* last_remote_lsn, within the structure for comparison with the current
* cycle's values. Considering the minimal cost of continuing in
- * RCI_WAIT_FOR_LOCAL_FLUSH without awaiting changes, we opt not to
- * advance transaction ID in this case.
+ * RCI_WAIT_FOR_LOCAL_FLUSH without awaiting changes, we opted not to
+ * advance the transaction ID here.
*/
if (FullTransactionIdPrecedesOrEquals(data->last_phase_at,
data->remote_oldestxid))
@@ -4399,7 +4394,9 @@ wait_for_local_flush(RetainConflictInfoData *data)
/*
* Reset all data fields except those used to determine the timing for
the
- * next round of transaction ID advancement.
+ * next round of transaction ID advancement. We can even use
+ * flushpos_update_time in the next round to decide whether to get the
+ * latest flush position.
*/
data->phase = RCI_GET_CANDIDATE_XID;
data->remote_lsn = InvalidXLogRecPtr;
diff --git a/src/backend/storage/ipc/procarray.c
b/src/backend/storage/ipc/procarray.c
index c1d5ddbad4b..ce87f40e986 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -2876,8 +2876,8 @@ GetRunningTransactionData(void)
* simple as possible and leave GetSnapshotData() as the primary code for
* that bookkeeping.
*
- * If inCommitOnly is true, we target transactions currently in the commit
- * phase only.
+ * inCommitOnly indicates getting the oldestActiveXid among the transactions
+ * in the commit critical section.
*/
TransactionId
GetOldestActiveTransactionId(bool inCommitOnly)
