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: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Nisha Moond <nisha.moond412@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, shveta malik <shveta.malik@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2024-12-11T07:33:32Z
Lists: pgsql-hackers 
On Wednesday, December 11, 2024 1:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Dec 6, 2024 at 1:28 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> > On Thursday, December 5, 2024 6:00 PM Amit Kapila
> <amit.kapila16@gmail.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?

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.

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.