On Tue, Jun 10, 2025 at 11:55 AM Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.com> wrote: > > Here is the V35 patch set which includes the following changes: >
Thank You for the patches, few comments: 1) compute_min_nonremovable_xid: + /* + * Stop advancing xmin if an invalid non-removable transaction ID is + * found, otherwise update xmin. + */ + if (!TransactionIdIsValid(nonremovable_xid)) + *can_advance_xmin = false; IMO, we can not have invalid nonremovable_xid here. It may be a possibility in a later patch where we do stop-conflict-detection for a worker and resets nonremovable_xid to Invalid. But in patch003, should this rather be an Assert as we want to ensure that worker's oldest is set to slot's-xmin by this time. 2) postgres=# create subscription sub1 connection 'dbname=postgres host=localhost user=shveta port=5433' publication pub1 WITH (retain_conflict_info = true); NOTICE: deleted rows will continue to accumulate for detecting conflicts until the subscription is enabled NOTICE: created replication slot "sub1" on publisher CREATE SUBSCRIPTION postgres=# alter subscription sub2 enable; NOTICE: deleted rows will continue to accumulate for detecting conflicts until the subscription is enabled We should not have this 'NOTICE: deleted rows' in above 2 commands. 3) + +ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE); +DROP SUBSCRIPTION regress_testsub; Shall we enable regress_testsub as well before we drop it to catch unexpected notices/warnings if any? 4) postgres=# alter subscription sub2 disable; WARNING: commit timestamp and origin data required for detecting conflicts won't be retained HINT: Consider setting "track_commit_timestamp" to true. Do we need "track_commit_timestamp" related WARNING while disabling rci sub as well? I feel it should be there while enabling it alone. thanks Shveta
