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: Nisha Moond <nisha.moond412@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, shveta malik <shveta.malik@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2024-11-27T10:56:09Z
Lists: pgsql-hackers 
On Tue, Nov 26, 2024 at 1:50 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>

Few comments on the latest 0001 patch:
1.
+ * - RCI_REQUEST_PUBLISHER_STATUS:
+ *   Send a message to the walsender requesting the publisher status, which
+ *   includes the latest WAL write position and information about running
+ *   transactions.

Shall we make the later part of this comment (".. information about
running transactions.") accurate w.r.t the latest changes of
requesting xacts that are known to be in the process of committing?

2.
+ * The overall state progression is: GET_CANDIDATE_XID ->
+ * REQUEST_PUBLISHER_STATUS -> WAIT_FOR_PUBLISHER_STATUS -> (loop to
+ * REQUEST_PUBLISHER_STATUS if concurrent remote transactions persist) ->
+ * WAIT_FOR_LOCAL_FLUSH.

This state machine progression misses to mention that after we waited
for flush the state again moves back to GET_CANDIDATE_XID.

3.
+request_publisher_status(RetainConflictInfoData *data)
+{
...
+ /* Send a WAL position request message to the server */
+ walrcv_send(LogRepWorkerWalRcvConn,
+ reply_message->data, reply_message->len);

This message requests more than a WAL write position but the comment
is incomplete.

4.
+/*
+ * Process the request for a primary status update message.
+ */
+static void
+ProcessStandbyPSRequestMessage(void)
...
+ /*
+ * Information about running transactions and the WAL write position is
+ * only available on a non-standby server.
+ */
+ if (!RecoveryInProgress())
+ {
+ oldestXidInCommit = GetOldestTransactionIdInCommit();
+ nextFullXid = ReadNextFullTransactionId();
+ lsn = GetXLogWriteRecPtr();
+ }

Shall we ever reach here for a standby case? If not shouldn't that be an ERROR?

-- 
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.