Horton

Re: Conflict detection for update_deleted in logical replication

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Nisha Moond <nisha.moond412@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, shveta malik <shveta.malik@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-01-01T12:24:54Z
Lists: pgsql-hackers

Attachments

On Wed, Dec 25, 2024 at 8:13 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Attach the new version patch set which addressed all other comments.
>

Review comments on 0001 and 0002
=============================
1.
/*
+ * Reset all data fields except those used to determine the timing for the
+ * next round of transaction ID advancement.
+ */
+ data->phase = RCI_GET_CANDIDATE_XID;
+ data->remote_lsn = InvalidXLogRecPtr;

There is no comment in the data-structure RetainConflictInfoData that
indicates the fields used to determine the timing for the next round
of transaction ID advancement. Can we add a comment in
RetainConflictInfoData to indicate the same?

2.
+ int xid_advancement_interval; /* how much time (ms) to wait
+ * before attempting to advance
+ * the non-removable transaction
+ * ID */
 } RetainConflictInfoData;

Shall we rename it to a bit simpler name xid_advance_interval? If you
agree with this change, we can probably rename
adjust_xid_advancement_interval() to adjust_xid_advance_interval() as
well.

3.
+/*
+ * The minimum (100ms) and maximum (3 minutes) intervals for advancing
+ * non-removable transaction IDs.
+ */
+#define MIN_XID_ADVANCEMENT_INTERVAL 100
+#define MAX_XID_ADVANCEMENT_INTERVAL 180000

Is there any reason to keep the maximum value as 3 minutes? If not
then mention that it is arbitrary and sufficient to not cause any
undue network traffic.

4.
@@ -4129,7 +4149,7 @@ get_candidate_xid(RetainConflictInfoData *data)
  * can consider the other interval or a separate GUC if the need arises.
  */
  if (!TimestampDifferenceExceeds(data->candidate_xid_time, now,
- wal_receiver_status_interval * 1000))
+ data->xid_advancement_interval))

The comment atop the above change in the second patch needs to change.

5.
+static void
+adjust_xid_advancement_interval(RetainConflictInfoData *data, bool
new_xid_found)

Let's move the location of this function to after
can_advance_nonremovable_xid(). This is to keep the functions to
transition the retain_data_phases together.

Apart from the above, I have made changes in a few comments in the
attached. Please include those after review and combine 0001 and 0002
as one patch.

-- 
With Regards,
Amit Kapila.

Commits

  1. Fix intermittent BF failures in 035_conflicts.

  2. Resume conflict-relevant data retention automatically.

  3. Fix intermittent test failure introduced in 6456c6e2c4.

  4. Fix Coverity issue reported in commit a850be2fe.

  5. Add test to prevent premature removal of conflict-relevant data.

  6. Post-commit review fixes for 228c370868.

  7. Add max_retention_duration option to subscriptions.

  8. Detect and report update_deleted conflicts.

  9. Preserve conflict-relevant data during logical replication.

  10. Integrate FullTransactionIds deeper into two-phase code

  11. Improve checks for GUC recovery_target_timeline

  12. Prevent excessive delays before launching new logrep workers.

  13. Keep WAL segments by slot's last saved restart LSN

  14. Rework some code handling pg_subscription data in psql and pg_dump

  15. Use generateClonedIndexStmt to propagate CREATE INDEX to partitions.