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