RE: Conflict detection for update_deleted in logical replication

On Wednesday, November 27, 2024 6:56 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Nov 26, 2024 at 1:50 PM Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)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