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: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Amit Kapila <amit.kapila16@gmail.com>
Cc: 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-25T02:43:02Z
Lists: pgsql-hackers

Attachments

On Monday, December 23, 2024 2:15 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> wrote:
> 
> Dear Hou,
> 
> Thanks for updating the patch. Few comments:

Thanks for the comments!

> 02.  ErrorOnReservedSlotName()
> 
> Currently the function is callsed from three points -
> create_physical_replication_slot(),
> create_logical_replication_slot() and CreateReplicationSlot().
> Can we move them to the ReplicationSlotCreate(), or combine into
> ReplicationSlotValidateName()?

I am not sure because moving the check into these functions because that would
prevent the launcher from creating the slot as well unless we add a new
parameter for these functions, but I am not sure if it's worth it at this
stage.

> 
> 03. advance_conflict_slot_xmin()
> 
> ```
> 	Assert(TransactionIdIsValid(MyReplicationSlot->data.xmin));
> ```
> 
> Assuming the case that the launcher crashed just after
> ReplicationSlotCreate(CONFLICT_DETECTION_SLOT).
> After the restart, the slot can be acquired since
> SearchNamedReplicationSlot(CONFLICT_DETECTION_SLOT)
> is true, but the process would fail the assert because data.xmin is still invalid.
> 
> I think we should re-create the slot when the xmin is invalid. Thought?

After thinking more, the standard approach to me would be to mark the slot as
EPHEMERAL during creation and persist it after initializing, so changed like
that.

> 05. check_remote_recovery()
> 
> Can we add a test case related with this?

I think the code path is already tested, and I am a bit unsure if we want to setup
a standby to test the ERROR case, so didn't add this.

---

Attach the new version patch set which addressed all other comments.

Based on some off-list discussions with Sawada-san and Amit, it would be better
if the apply worker can avoid reporting an ERROR if the publisher's clock's
lags behind that of the subscriber, so I implemented a new 0007 patch to allow
the apply worker to wait for the clock skew to pass and then send a new request
to the publisher for the latest status. The implementation is as follows:

Since we have the time (reply_time) on the walsender when it confirms that all
the committing transactions have finished, it means any subsequent transactions
on the publisher should be assigned a commit timestamp later then reply_time.
And the (candidate_xid_time) when it determines the oldest active xid. Any old
transactions on the publisher that have finished should have a commit timestamp
earlier than the candidate_xid_time.

The apply worker can compare the candidate_xid_time with reply_time. If
candidate_xid_time is less than the reply_time, then it's OK to advance the xid
immdidately. If candidate_xid_time is greater than reply_time, it means the
clock of publisher is behind that of the subscriber, so the apply worker can
wait for the skew to pass before advancing the xid.

Since this is considered as an improvement, we can focus on this after
pushing the main patches.

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.