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-19T11:04:38Z
Lists: pgsql-hackers 
>
> Here is the V38 patch set which includes the following changes:
>

Thank You for the patches. Few comments:

1)
+   <para>
+    Note that commit timestamps and origin data retained by enabling the
+    <link linkend="sql-createsubscription-params-with-retain-conflict-info"><literal>retain_conflict_info</literal></link>
+    option will not be preserved during the upgrade. As a
+    result, the upgraded subscriber might be unable to detect conflicts or log
+    relevant commit timestamps and origins when applying changes from the
+    publisher occurring during the upgrade.
+   </para>

This statement is true even for changes pending from 'before' the
upgrade.  So we shall change last line where we mention 'during the
upgrade'

2)
+         <para>
+          Note that the information for conflict detection cannot be purged if
+          the subscription is disabled; thus, the information will accumulate
+          until the subscription is enabled. To prevent excessive accumulation,
+          it is recommended to disable <literal>retain_conflict_info</literal>
+          if the subscription will be inactive for an extended period.
+         </para>

I think this can be put in WARNING or CAUTION tags as this is
something which if neglected can result in system bloat.

3)
postgres=# create subscription sub3 connection 'dbname=postgres
host=localhost user=shveta port=5433' publication pub2 WITH (failover
= true, retain_conflict_info = true);
WARNING:  commit timestamp and origin data required for detecting
conflicts won't be retained
HINT:  Consider setting "track_commit_timestamp" to true.
ERROR:  subscription "sub3" already exists

In CreateSubscription(), we shall move CheckSubConflictInfoRetention()
after sub-duplicity check. Above WARNING with the existing-sub ERROR
looks odd.

4)
In check_new_cluster_replication_slots(), earlier we were not doing
any checks for 'count of logical slots on new cluster' if there were
no logical slots on old cluster (i.e. nslots_on_old is 0). Now we are
doing a 'nslots_on_new' related check even when 'nslots_on_old' is 0
for the case when RCI is enabled. Shouldn't we skip 'nslots_on_new'
check when 'nslots_on_old' is 0?

5)
We refer to 'update_deleted' in patch1's comment when the conflict is
not yet created. Is it okay?

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.