From de523a1f5304bec7418eaf2d5c6c2671cf513606 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Fri, 3 Jul 2026 14:46:42 +0000 Subject: [PATCH v6 4/4] Add invalidation-based retry loop for AlterPublication Apply the same RangeVarGetRelidExtended() style retry loop to AlterPublication()'s tables/schemas branch that was added for subscriptions in the preceding XXX commit. Previously, this branch resolved the publication name and checked ownership at the top of AlterPublication(), then locked and re-read by OID. This left a window where concurrent DDL could have modified the ownership and/or the name resolution. Now the tables/schemas branch has its own complete retry loop: name resolution, ownership check, and lock acquisition all inside the loop. Add an isolation test that changes the publication owner while an ALTER PUBLICATION command is waiting for the publication object lock, and verifies that the former owner is rejected when the command resumes. Author: Bertrand Drouvot Reviewed-by: Dilip Kumar Reviewed-by: Hayato Kuroda (Fujitsu) Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg --- src/backend/commands/publicationcmds.c | 105 ++++++++++++------ .../expected/publication-owner-locking.out | 12 ++ src/test/isolation/isolation_schedule | 1 + .../specs/publication-owner-locking.spec | 37 ++++++ 4 files changed, 122 insertions(+), 33 deletions(-) 60.5% src/backend/commands/ 12.3% src/test/isolation/expected/ 26.5% src/test/isolation/specs/ diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 440adb356ad..dfd707bc7d7 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -39,6 +39,7 @@ #include "parser/parse_relation.h" #include "rewrite/rewriteHandler.h" #include "storage/lmgr.h" +#include "storage/sinval.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/inval.h" @@ -1662,54 +1663,92 @@ AlterPublication(ParseState *pstate, AlterPublicationStmt *stmt) rel = table_open(PublicationRelationId, RowExclusiveLock); - tup = SearchSysCacheCopy1(PUBLICATIONNAME, - CStringGetDatum(stmt->pubname)); + if (stmt->options) + { + tup = SearchSysCacheCopy1(PUBLICATIONNAME, + CStringGetDatum(stmt->pubname)); - if (!HeapTupleIsValid(tup)) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("publication \"%s\" does not exist", - stmt->pubname))); + if (!HeapTupleIsValid(tup)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("publication \"%s\" does not exist", + stmt->pubname))); - pubform = (Form_pg_publication) GETSTRUCT(tup); + pubform = (Form_pg_publication) GETSTRUCT(tup); - /* must be owner */ - if (!object_ownercheck(PublicationRelationId, pubform->oid, GetUserId())) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION, - stmt->pubname); + /* must be owner */ + if (!object_ownercheck(PublicationRelationId, pubform->oid, + GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION, + stmt->pubname); - if (stmt->options) AlterPublicationOptions(pstate, stmt, rel, tup); + } else { List *relations = NIL; List *exceptrelations = NIL; List *schemaidlist = NIL; - Oid pubid = pubform->oid; + Oid pubid; - ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations, - &exceptrelations, &schemaidlist); + /* + * Lock the publication so nobody else can do anything with it. + * + * Like RangeVarGetRelidExtended() does for relations, we resolve the + * name, check ownership, and lock inside a loop. If invalidation + * messages arrive (indicating concurrent DDL), we retry. We keep the + * lock held across retries and only release it if the name resolves + * to a different OID on the next iteration. + */ + { + Oid oldPubId = InvalidOid; + bool retry = false; - CheckAlterPublication(stmt, tup, relations, schemaidlist); + for (;;) + { + uint64 inval_count = SharedInvalidMessageCounter; - heap_freetuple(tup); + tup = SearchSysCacheCopy1(PUBLICATIONNAME, + CStringGetDatum(stmt->pubname)); - /* Lock the publication so nobody else can do anything with it. */ - LockDatabaseObject(PublicationRelationId, pubid, 0, - AccessExclusiveLock); + if (!HeapTupleIsValid(tup)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("publication \"%s\" does not exist", + stmt->pubname))); - /* - * It is possible that by the time we acquire the lock on publication, - * concurrent DDL has removed it. We can test this by checking the - * existence of publication. We get the tuple again to avoid the risk - * of any publication option getting changed. - */ - tup = SearchSysCacheCopy1(PUBLICATIONOID, ObjectIdGetDatum(pubid)); - if (!HeapTupleIsValid(tup)) - ereport(ERROR, - errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("publication \"%s\" does not exist", - stmt->pubname)); + pubform = (Form_pg_publication) GETSTRUCT(tup); + pubid = pubform->oid; + + if (!object_ownercheck(PublicationRelationId, pubid, + GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION, + stmt->pubname); + + if (retry) + { + if (pubid == oldPubId) + break; + UnlockDatabaseObject(PublicationRelationId, oldPubId, 0, + AccessExclusiveLock); + } + + LockDatabaseObject(PublicationRelationId, pubid, 0, + AccessExclusiveLock); + + if (inval_count == SharedInvalidMessageCounter) + break; + + retry = true; + oldPubId = pubid; + heap_freetuple(tup); + } + } + + ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations, + &exceptrelations, &schemaidlist); + + CheckAlterPublication(stmt, tup, relations, schemaidlist); relations = list_concat(relations, exceptrelations); AlterPublicationTables(stmt, tup, relations, pstate->p_sourcetext, diff --git a/src/test/isolation/expected/publication-owner-locking.out b/src/test/isolation/expected/publication-owner-locking.out new file mode 100644 index 00000000000..06d1d70d5a9 --- /dev/null +++ b/src/test/isolation/expected/publication-owner-locking.out @@ -0,0 +1,12 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_begin s1_lock s1_alter_owner s2_set_role s2_alter s1_commit s2_reset_role +step s1_begin: BEGIN; +step s1_lock: COMMENT ON PUBLICATION regress_pub_owner_lock IS 'locked'; +step s1_alter_owner: ALTER PUBLICATION regress_pub_owner_lock OWNER TO regress_pub_owner2; +step s2_set_role: SET ROLE regress_pub_owner1; +step s2_alter: ALTER PUBLICATION regress_pub_owner_lock ADD TABLE regress_pub_owner_lock_table; +step s1_commit: COMMIT; +step s2_alter: <... completed> +ERROR: must be owner of publication regress_pub_owner_lock +step s2_reset_role: RESET ROLE; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index eb1b257e56e..fb60d106f7f 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -129,5 +129,6 @@ test: lock-nowait test: for-portion-of test: ddl-dependency-locking test: subscription-owner-locking +test: publication-owner-locking test: pub-concurrent-drop test: drop-owned-grant diff --git a/src/test/isolation/specs/publication-owner-locking.spec b/src/test/isolation/specs/publication-owner-locking.spec new file mode 100644 index 00000000000..8ef0e8c31b7 --- /dev/null +++ b/src/test/isolation/specs/publication-owner-locking.spec @@ -0,0 +1,37 @@ +# Test post-lock publication ownership checks in ALTER PUBLICATION. +# +# Session s1 holds the publication object lock with COMMENT ON PUBLICATION, +# then changes the owner in the same transaction. Session s2 sees the old +# owner and waits for the object lock. Once s1 commits, s2 must recheck the +# publication state and reject the former owner. + +setup +{ + CREATE ROLE regress_pub_owner1; + CREATE ROLE regress_pub_owner2; + CREATE TABLE regress_pub_owner_lock_table (a int); + ALTER TABLE regress_pub_owner_lock_table OWNER TO regress_pub_owner1; + CREATE PUBLICATION regress_pub_owner_lock; + ALTER PUBLICATION regress_pub_owner_lock OWNER TO regress_pub_owner1; +} + +teardown +{ + DROP PUBLICATION regress_pub_owner_lock; + DROP TABLE regress_pub_owner_lock_table; + DROP ROLE regress_pub_owner1; + DROP ROLE regress_pub_owner2; +} + +session s1 +step s1_begin { BEGIN; } +step s1_lock { COMMENT ON PUBLICATION regress_pub_owner_lock IS 'locked'; } +step s1_alter_owner { ALTER PUBLICATION regress_pub_owner_lock OWNER TO regress_pub_owner2; } +step s1_commit { COMMIT; } + +session s2 +step s2_set_role { SET ROLE regress_pub_owner1; } +step s2_alter { ALTER PUBLICATION regress_pub_owner_lock ADD TABLE regress_pub_owner_lock_table; } +step s2_reset_role { RESET ROLE; } + +permutation s1_begin s1_lock s1_alter_owner s2_set_role s2_alter s1_commit s2_reset_role -- 2.34.1