agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Subject: [PATCH v6 4/4] Add invalidation-based retry loop for AlterPublication
Date: Fri, 3 Jul 2026 14:46:42 +0000
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
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; <waiting ...>
+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
--YgvQoaaPJLlueFuQ--
Message-ID: <no-message-id-1192100@localhost>
Permalink: ../no-message-id-1192100@localhost/
Also on: postgresql.org/message-id/no-message-id-1192100@localhost
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bertranddrouvot.pg@gmail.com
Subject: Re: [PATCH v6 4/4] Add invalidation-based retry loop for AlterPublication
In-Reply-To: <no-message-id-1192100@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox