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: Amit Kapila <amit.kapila16@gmail.com>, 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>
Date: 2025-06-13T06:52:10Z
Lists: pgsql-hackers 
On Thu, Jun 12, 2025 at 4:22 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Jun 12, 2025 at 11:34 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> >
> > Here is the V35 patch set which includes the following changes:
> >
>
> Thank You for the patches. Few comments:
>
> 1)
> Since now we create slot for rci enabled subscription, it will require
> wal_level >= replica even on subscribers. We shall mention this in the
> docs.
>
> 2)
> postgres=# alter subscription sub3 set (retain_conflict_info=true);
> NOTICE:  deleted rows will continue to accumulate for detecting
> conflicts until the subscription is enabled
> ALTER SUBSCRIPTION
>
> postgres=# alter subscription sub3 disable;
> WARNING:  deleted rows to detect conflicts would not be removed until
> the subscription is enabled
> HINT:  Consider setting retain_conflict_info to false.
>
> I feel we shall have the same message in both the cases as we are
> trying to convey the exact same thing concerning deleted rows
> accumulation..
>

Few more comments:

3)

+static void
+create_conflict_slot_if_not_exists(void)
+{
- /* Exit early if the replication slot is already created and acquired */
+ /* Exit early, if the replication slot is already created and acquired */
  if (MyReplicationSlot)
  return;

- /* If the replication slot exists, acquire it and exit */
+ /* If the replication slot exists, acquire it, and exit */
  if (acquire_conflict_slot_if_exists())
  return;

We never release the slot explicitly in the launcher and thus do not
need 'If the replication slot exists, acquire it' code part here. For
the cases 1) when slot is dropped and recreated 2) slot is acquired by
launcher on restart; 'if (MyReplicationSlot)' check is enough.

4)
/*
 * Common checks for altering failover and two_phase options.
 */
 @@ -1051,7 +1075,8 @@ CheckAlterSubOption(Subscription *sub, const
char *option,
  * two_phase options.
  */
  Assert(strcmp(option, "failover") == 0 ||
-    strcmp(option, "two_phase") == 0);
+    strcmp(option, "two_phase") == 0 ||
+    strcmp(option, "retain_conflict_info") == 0);

Please update the comment atop CheckAlterSubOption to mention
retain_conflict_info as well.

5)
In CheckAlterSubOption(), we need to ensure that when
'slot_needs_update' is true, it is either failover or two_phase but
not rci. Such Assert can be added.

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.