Re: Conflict detection for update_deleted in logical replication

On Fri, Dec 6, 2024 at 1:28 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Thursday, December 5, 2024 6:00 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> >
> > 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.
>

As part of the above comment, I had asked for three things (a) avoid
setting data->remote_lsn = InvalidXLogRecPtr; in
request_publisher_status(); (b) avoid setting data->last_phase_at
=InvalidFullTransactionId; in get_candidate_xid(); (c) reset data in
wait_for_local_flush() after wait is over. You only did (a) in the
patch and didn't mention anything about (b) or (c). Is that
intentional? If so, what is the reason?

*
+static bool
+can_advance_nonremovable_xid(RetainConflictInfoData *data)
+{
+

Isn't it better to make this an inline function as it contains just one check?

*
+ /*
+ * The non-removable transaction ID for a subscription is centrally
+ * managed by the main apply worker.
+ */
+ if (!am_leader_apply_worker())

I have tried to improve this comment in the attached.

--
With Regards,
Amit Kapila.