Re: Conflict detection for update_deleted in logical replication

On Thu, Jan 23, 2025 at 5:17 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
I was reviewing v26 patch set and have some comments so far I reviewed
0001 so most of the comments/question are from this patch.

comments on v26-0001

1.
+ next_full_xid = ReadNextFullTransactionId();
+ epoch = EpochFromFullTransactionId(next_full_xid);
+
+ /*
+ * Adjust the epoch if the next transaction ID is less than the oldest
+ * running transaction ID. This handles the case where transaction ID
+ * wraparound has occurred.
+ */
+ if (oldest_running_xid > XidFromFullTransactionId(next_full_xid))
+ epoch--;
+
+ full_xid = FullTransactionIdFromEpochAndXid(epoch, oldest_running_xid);

I think you can directly use the 'AdjustToFullTransactionId()'
function here, maybe we can move that somewhere else and make that
non-static function.

2.
+ /*
+ * We expect the publisher and subscriber clocks to be in sync using time
+ * sync service like NTP. Otherwise, we will advance this worker's
+ * oldest_nonremovable_xid prematurely, leading to the removal of rows
+ * required to detect update_delete conflict.
+ *
+ * XXX Consider waiting for the publisher's clock to catch up with the
+ * subscriber's before proceeding to the next phase.
+ */
+ if (TimestampDifferenceExceeds(data->reply_time,
+ data->candidate_xid_time, 0))
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("oldest_nonremovable_xid transaction ID may be advanced prematurely"),
+ errdetail("The clock on the publisher is behind that of the subscriber."));

I don't fully understand the purpose of this check. Based on the
comments in RetainConflictInfoData, if I understand correctly,
candidate_xid_time represents the time when the candidate is
determined, and reply_time indicates the time of the reply from the
publisher. Why do we expect these two timestamps to have zero
difference to ensure clock synchronization?

3.
+ /*
+ * Use last_recv_time when applying changes in the loop; otherwise, get
+ * the latest timestamp.
+ */
+ now = data->last_recv_time ? data->last_recv_time : GetCurrentTimestamp();

Can you explain in the comment what's the logic behind using
last_recv_time here? Why not just compare 'candidate_xid_time' vs
current timestamp?

4.
Comment of v26-0004 doesn't clearly explain that once retention
stopped after reaching 'max_conflict_retention_duration' will it
resume back?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com