Horton

RE: Conflict detection for update_deleted in logical replication

Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>

View thread View on postgr.es
From: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
To: Amit Kapila <amit.kapila16@gmail.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-29T02:24:37Z
Lists: pgsql-hackers

Attachments

On Wednesday, November 27, 2024 6:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> 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?

Changed.

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

Added.

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

Thanks for pointing, improved.

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

It is possible to reach here if user creates a subscription with
(connect=false,detect_update_deleted=true), in which case we could not check it
during creation. But I agree that it would be better to report an ERROR here,
so changed as suggested. After this change, there is no need to check the
invalid remote lsn in apply worker and thus the error can also be removed.

I also modified the 0004 patch to check the remote server's recovery during
"alter subscription enable and alter subscription connect", so that it can avoid
sending a request to the standby in apply worker.


Attached is the V12 patch set.

In addition to addressing the previous comments, I have introduced a new flag,
DELAY_CHKPT_IN_COMMIT, within delayChkptFlags. This flag is marked in
RecordTransactionCommit(). This is because the existing DELAY_CHKPT_START is
used in other places such as PREPARE where we don't have to wait, so relying on
it might delay the advancement of non-removable xid due to these un-related
txns.

Best Regards,
Hou zj

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.