RE: Conflict detection for update_deleted in logical replication

On Thursday, December 5, 2024 6:00 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Dec 4, 2024 at 4:29 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> >
> > 1.
> > + if (can_advance_nonremovable_xid(&data, last_recv_timestamp))
> > + maybe_advance_nonremovable_xid(&data);
> >
> > In can_advance_nonremovable_xid(), we determine whether to advance the
> > oldest xid based on 'last_recv_timestamp' and then again in
> > maybe_advance_nonremovable_xid()->get_candidate_xid(), we compare it
> > with the current time. How does that make sense? Shall we use
> > 'last_recv_timestamp' directly in get_candidate_xid() as that will
> > avoid the additional time check in can_advance_nonremovable_xid()?

Agreed.

I added a field to RetainConflictInfoData to store the last_recv_timestamp and
am using it directly in get_candidate_xid(). If no message has been received,
we can still obtain the current timestamp.

> >
> > 2.
> > + TimestampTz next_attempt_time; /* when to attemp to advance the xid
> > +during
> > + * change application */
> > +} RetainConflictInfoData;
> >
> > This new variable introduced in this version is not used in the patch.
> > Any reason or just a leftover?

It was a leftover, removed.

> >
> > Apart from the above, I have made a few updates in the comments in the
> > attached. Please include those after review.
> >
>
> A few more comments:
> 1.
> +static void
> +wait_for_local_flush(RetainConflictInfoData *data)
> {
> ...
> +
> + data->phase = RCI_GET_CANDIDATE_XID;
> +
> + maybe_advance_nonremovable_xid(data);
> +}
>
> Isn't it better to reset all the fields of data before the next round of
> GET_CANDIDATE_XID phase? If we do that then we don't need to reset
> data->remote_lsn = InvalidXLogRecPtr; and data->last_phase_at =
> InvalidFullTransactionId; individually in request_publisher_status() and
> get_candidate_xid() respectively. Also, it looks clean and logical to me unless I
> am missing something.

The remote_lsn was used to determine whether a status is received, so was reset
each time in request_publisher_status. To make it more straightforward, I added
a new function parameter 'status_received', which would be set to true when
calling maybe_advance_nonremovable_xid() on receving the status. After this
change, there is no need to reset the remote_lsn.

>
> 2.
> + /*
> + * Issue a warning if there is a detected clock skew between the
> + publisher
> + * and subscriber.
> + *
> + * 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(WARNING,
> + errmsg("non-removable transaction ID may be advanced prematurely"),
> + errdetail("The clock on the publisher is behind that of the
> + subscriber."));
>
> Shouldn't this be an ERROR as this will lead to the removal of rows required to
> detect update_delete conflict?

Agreed. Changed to ERROR.

>
> Apart from the above, I have made a few more updates in the comments in the
> attached. Please include those after review.

Thanks, I have checked and merged.

Here is V15 patch set which addressed above comments. I also fixed a bug that
the table sync worker mis-sent the request to publisher.

Best Regards,
Hou zj