Re: Conflict detection for update_deleted in logical replication

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