RE: Conflict detection for update_deleted in logical replication

On Wednesday, December 11, 2024 3:34 PM Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Wednesday, December 11, 2024 1:06 PM Amit Kapila
> <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > 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();
> > data->(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?
>
> I think I misunderstood the intention, so will address in next version.
>
> >
> > *
> > +static bool
> > +can_advance_nonremovable_xid(RetainConflictInfoData *data) {
> > +
> >
> > Isn't it better to make this an inline function as it contains just one check?
>
> Agreed. Will address in next version.
>
> >
> > *
> > + /*
> > + * 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.
>
> Thanks, will check and merge the next version.

Attach the V16 patch set which addressed above comments.

There is a new 0002 patch where I tried to dynamically adjust the interval for
advancing the transaction ID. Instead of always waiting for
wal_receiver_status_interval, we can start with a short interval and increase
it if there is no activity (no xid assigned on subscriber), but not beyond
wal_receiver_status_interval.

The intention is to more effectively advance xid to avoid retaining too much
dead tuples. My colleague will soon share detailed performance data and
analysis related to this enhancement.

Best Regards,
Hou zj