On Thu, Jun 19, 2025 at 4:34 PM shveta malik <shveta.ma...@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'
>
> 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?
>
Please find few more comments:
6)
We can add in doc that pg_conflict_detection is a physical slot with
no wals-reserved.
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.
thanks
Shveta
