On Wed, Jun 17, 2026 at 4:29 PM Nisha Moond <[email protected]> wrote: > > On Wed, Jun 17, 2026 at 2:49 PM Ajin Cherian <[email protected]> wrote: > > > > I have addressed the comments in v8 (attached). I've also now taken > > out the check for PG17 on old cluster and now all clusters which have > > replication origins are migrated. > > Hi Ajin, Thanks for the patches. > I have started reviewing the patches. Please find a few initial > comments below (from v7 review, still applicable to v8):
Please find a couple more comments for the v8-002 patch: 4) I was able to build successfully without all three new header inclusions below, so we may not need them. --- a/src/backend/utils/adt/pg_upgrade_support.c +++ b/src/backend/utils/adt/pg_upgrade_support.c @@ -11,6 +11,7 @@ +#include "access/genam.h" ... +#include "utils/fmgroids.h" ... +#include "utils/snapmgr.h" ~~~ 5) Race condition in origin.c: replorigin_create() - heap_freetuple(tuple); + table_close(rel, ExclusiveLock); + + replorigin_create_with_id(roident, roname, InvalidXLogRecPtr); + Due to the above refactoring, CREATE SUBSCRIPTION can now fail because of a race between two concurrent CREATE SUBSCRIPTION commands: postgres=# create subscription sub2 connection 'dbname=postgres host=localhost port=7733' publication pub1; ERROR: replication origin with ID 2 already exists I think this happens because the table lock is released, allowing another session to create a pg_origin with the next available roident, which replorigin_create_with_id() later attempts to use. Would it make sense to call replorigin_create_with_id() before table_close() so the lock is still held? I tested this and it works, though I'm not fully sure whether self-locking could be a concern here, since replorigin_create_with_id() again acquires a RowExclusiveLock on the same table. ~~~ -- Thanks, Nisha
