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>, Masahiko Sawada <sawada.mshk@gmail.com>, shveta malik <shveta.malik@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2024-10-21T03:09:59Z
Lists: pgsql-hackers

Attachments

On Friday, October 18, 2024 5:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Fri, Oct 11, 2024 at 2:04 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> >
> > Attach the V4 patch set which addressed above comments.
> >
> 
> A few minor comments:

Thanks for the comments.

> 1.
> + * Retaining the dead tuples for this period is sufficient because any
> + * subsequent transaction from the publisher will have a later timestamp.
> + * Therefore, it is acceptable if dead tuples are removed by vacuum and
> +an
> + * update_missing conflict is detected, as the correct resolution for
> +the
> + * last-update-wins strategy in this case is to convert the UPDATE to
> +an INSERT
> + * and apply it anyway.
> + *
> + * The 'remote_wal_pos' will be reset after sending a new request to
> walsender.
> + */
> +static void
> +maybe_advance_nonremovable_xid(XLogRecPtr *remote_wal_pos,
> +    DeadTupleRetainPhase *phase)
> 
> We should cover the key point of retaining dead tuples which is to avoid
> converting updates to inserts (considering the conflict as
> update_missing) in the comments above and also in the commit message.

Right, I updated the comments the commit messages for the same.

> 
> 2. In maybe_advance_nonremovable_xid() all three phases are handled by
> different if blocks but as per my understanding the phase value will be unique
> in one call to the function. So, shouldn't it be handled with else if?

I thought that it is possible that we can immediately pass the check in next phase,
thus, wanted to run the codes of next phase as well during one function call.

For example, after switching from DTR_WAIT_FOR_LOCAL_FLUSH to
DTR_REQUEST_WALSENDER_WAL_POS, it’s possible that enough time have passed since
the last wal position request, so It was intended to let it immediately send
the next request in the next if block if possible.

I added some comments in this version to make it clear.

Here is the V5 patch set which addressed above comments.

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.