Horton

Re: Conflict detection for update_deleted in logical replication

shveta malik <shveta.malik@gmail.com>

View thread View on postgr.es
From: shveta malik <shveta.malik@gmail.com>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: vignesh C <vignesh21@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, shveta malik <shveta.malik@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>
Date: 2025-05-20T03:08:39Z
Lists: pgsql-hackers 
Please find few more comments:

1)
ProcessStandbyPSRequestMessage:
+ /*
+ * This shouldn't happen because we don't support getting primary status
+ * message from standby.
+ */
+ if (RecoveryInProgress())
+ elog(ERROR, "the primary status is unavailable during recovery");


a) This error is not clear. Is it supposed to be user oriented error
or internal error? As a user, it is difficult to interpret this error
and take some action.

b) What I understood is that there is no user of enabling
'retain_conflict_info' for a subscription which is subscribing to a
publisher which is physical standby too. So shall we emit such an
ERROR during 'Create Sub(retain_conflict_info=on)' itself? But that
would need checking whether the publisher is physical standby or not
during CREATE-SUB. Is that a possibility?

2)

----------
  send_feedback(last_received, requestReply, requestReply);

+ maybe_advance_nonremovable_xid(&data, false);
+
  /*
  * Force reporting to ensure long idle periods don't lead to
  * arbitrarily delayed stats. Stats can only be reported outside
----------

Why do we need this call to 'maybe_advance_nonremovable_xid' towards
end of LogicalRepApplyLoop() i.e. the last call? Can it make any
further 'data.phase' change here? IIUC, there are 2 triggers for
'data.phase' change through LogicalRepApplyLoop(). First one is for
the very first time when we start this loop, it will set data.phase to
0  (RCI_GET_CANDIDATE_XID) and will call
maybe_advance_nonremovable_xid(). After that, LogicalRepApplyLoop()
function can trigger a 'data.phase' change only when it receives a
response from the publisher. Shouldn't the first 4 calls
 to maybe_advance_nonremovable_xid() from LogicalRepApplyLoop() suffice?

3)
Code is almost the same for GetOldestActiveTransactionId() and
GetOldestTransactionIdInCommit(). I think we can merge these two.
GetOldestActiveTransactionId() can take new arg "getTxnInCommit".
GetOldestTransactionIdInCommit() can call
GetOldestActiveTransactionId() with that arg as true, whereas other 2
callers can pass it as false.

thanks
Shveta

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.