RE: Conflict detection for update_deleted in logical replication

On Fri, Jun 20, 2025 at 3:22 PM shveta malik wrote:
> 
> On Thu, Jun 19, 2025 at 4:34 PM shveta malik <shveta.malik@gmail.com>
> wrote:
> >
> > >
> > > 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'

Changed.

> >
> > 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.

Agreed, I chose CAUTION since the replication slot page[1]
is using the same for the risk of WAL 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.

Changed.

> >
> > 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?

I agreed. In addition, I have also added a check to confirm the reserved
replication slot pg_conflict_detection does not exist on the new cluster
when migrating subscriptions with retain_conflict_info enabled.

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

Moved to later patches where update_deleted is supported.

> >
> 
> Please find few more comments:
> 
> 6)
> We can add in doc that pg_conflict_detection is a physical slot with no
> wals-reserved.

I added the physical mark. Since we have explicitly mention the info we are
retaining, I didn't mention WAL in this version.

> 
> 7)
> We shall error or give warning (whatever appropriate) in
> ReplicationSlotAcquire() (similar to ReplicationSlotValidateName()), that if it is
> pg_conflict_detection slot, then acquire is possible only if the process is
> launcher. This will prevent:
> 
> a) manual/accidental drop of slot by user before launcher could acquire it.
> b) usage of slot in primary_slot_name before launcher could acquire it.
> 
> It will also make  lot-advance error more meaningful. Currently it gives below
> error:
> 
> postgres=# select pg_replication_slot_advance ('pg_conflict_detection',
> pg_current_wal_lsn());
> ERROR:  replication slot "pg_conflict_detection" cannot be advanced
> DETAIL:  This slot has never previously reserved WAL, or it has been
> invalidated.

Added a new error message in slot acquisition as suggested.

Here is the V39 patch set which includes the following changes:

0001:
* Addressed the comments above.

0002:
* Addressed the comments above.
* Improved some comments and documentation additionally.

0003:
A new patch to migrate the conflict detection during upgrade. (Suggested by Amit[2])

0004:
Rebased

0005:
Rebased

0006:
Rebased

0007:
Rebased

0008:
Rebased

[1] https://www.postgresql.org/docs/devel/warm-standby.html#STREAMING-REPLICATION-SLOTS
[2] https://www.postgresql.org/message-id/CAA4eK1%2ByP3%2B4CRtkqPEn5DgsaQXJogep4QyqTVSaqk%3DouGqQEQ%40mail.gmail.com

Best Regards,
Hou zj