database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2026-10-23 13:29:09 +03:00
Code Activity

Validate ALTER PUBLICATION after acquiring the publication lock.

Browse Source 
AlterPublicationOptions() can use stale publication state when the
publication is modified concurrently while waiting for the publication
lock. In particular, a concurrent ALTER PUBLICATION ... SET ALL TABLES
can change puballtables, causing the validation to make decisions based
on the old value and leads to assertion failure. Likewise,
CheckAlterPublication() could miss a table added concurrently, letting
SET ALL TABLES EXCEPT (...) leave pg_publication_rel with a mixture of
inclusion and exclusion rows.

Fix by acquiring the lock and re-reading the publication tuple in
AlterPublication() before either path runs, and by calling
CheckAlterPublication() only after that.

Also fix 037_except.pl, which left one computed result untested and
silently reused a subscription created by an earlier test.

Author: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/CALDaNm1r2MkGu6h8zgU1Kj1sX-FcMQ7wGTeLSnhx-5joiyXEvg@mail.gmail.com
Backpatch-through: 19, where it was introduced
This commit is contained in:
Amit Kapila
2026-09-11 09:48:51 +05:30
parent c58566d06f
commit f6b2d6b827
2 changed files with 53 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

77
src/backend/commands/publicationcmds.c
View File

@@ -1057,15 +1057,6 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt,
if (!pubform->puballtables && publish_via_partition_root_given &&
!publish_via_partition_root)
{
/*
* Lock the publication so nobody else can do anything with it. This
* prevents concurrent alter to add partitioned table(s) with WHERE
* clause(s) and/or column lists which we don't allow when not
* publishing via root.
*/
LockDatabaseObject(PublicationRelationId, pubform->oid, 0,
AccessShareLock);
root_relids = GetIncludedPublicationRelations(pubform->oid,
PUBLICATION_PART_ROOT);
@@ -1659,6 +1650,10 @@ AlterPublication(ParseState *pstate, AlterPublicationStmt *stmt)
Relation rel;
HeapTuple tup;
Form_pg_publication pubform;
List *relations = NIL;
List *exceptrelations = NIL;
List *schemaidlist = NIL;
Oid pubid;
rel = table_open(PublicationRelationId, RowExclusiveLock);
@@ -1678,39 +1673,49 @@ AlterPublication(ParseState *pstate, AlterPublicationStmt *stmt)
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION,
stmt->pubname);
pubid = pubform->oid;
/*
* Resolve publication objects to OIDs when altering the publication
* objects.
*/
if (!stmt->options)
ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations,
&exceptrelations, &schemaidlist);
heap_freetuple(tup);
/*
* Lock the publication while we validate and update it. This prevents
* concurrent changes to the publication's relation and schema set, such
* as adding partitioned table(s) with WHERE clause(s) and/or column
* lists, which are not allowed when not publishing via root. It also
* ensures that the publication definition does not change via SET ALL
* TABLES between the validation performed by AlterPublicationOptions()
* and the subsequent catalog update.
*/
LockDatabaseObject(PublicationRelationId, pubid, 0,
stmt->options ? AccessShareLock : AccessExclusiveLock);
/*
* 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));
if (stmt->options)
AlterPublicationOptions(pstate, stmt, rel, tup);
else
{
List *relations = NIL;
List *exceptrelations = NIL;
List *schemaidlist = NIL;
Oid pubid = pubform->oid;
ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations,
&exceptrelations, &schemaidlist);
CheckAlterPublication(stmt, tup, relations, schemaidlist);
heap_freetuple(tup);
/* Lock the publication so nobody else can do anything with it. */
LockDatabaseObject(PublicationRelationId, pubid, 0,
AccessExclusiveLock);
/*
* 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));
relations = list_concat(relations, exceptrelations);
AlterPublicationTables(stmt, tup, relations, pstate->p_sourcetext,
schemaidlist != NIL);

21
src/test/subscription/t/037_except.pl
View File

@@ -52,6 +52,10 @@ sub test_except_root_partition
$result = $node_publisher->safe_psql('postgres',
"SELECT count(*) = 0 FROM pg_logical_slot_get_binary_changes('test_slot', NULL, NULL, 'proto_version', '1', 'publication_names', 'tap_pub_part')"
);
is($result, qq(t),
"no changes for the partitioned table in the EXCEPT clause are present in the replication slot (publish_via_partition_root = $pubviaroot)"
);
$node_publisher->wait_for_catchup('tap_sub_part');
# Verify that no rows are replicated to subscriber for root or partitions.
@@ -235,7 +239,7 @@ $node_publisher->safe_psql(
CREATE PUBLICATION tap_pub2 FOR TABLE tab1;
INSERT INTO tab1 VALUES(1);
));
$node_subscriber->psql('postgres',
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub1, tap_pub2"
);
$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub');
@@ -249,12 +253,17 @@ is( $result, qq(1
2),
"check replication of a table in the EXCEPT clause of one publication but included by another"
);
$node_subscriber->safe_psql(
'postgres', qq(
DROP SUBSCRIPTION tap_sub;
TRUNCATE tab1;
));
$node_publisher->safe_psql(
'postgres', qq(
DROP PUBLICATION tap_pub2;
TRUNCATE tab1;
));
$node_subscriber->safe_psql('postgres', qq(TRUNCATE tab1));
# OK when a table is excluded by pub1 EXCEPT clause, but it is included by pub2
# FOR ALL TABLES.
@@ -263,7 +272,7 @@ $node_publisher->safe_psql(
CREATE PUBLICATION tap_pub2 FOR ALL TABLES;
INSERT INTO tab1 VALUES(1);
));
$node_subscriber->psql('postgres',
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub1, tap_pub2"
);
$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub');
@@ -278,10 +287,4 @@ is( $result, qq(1
"check replication of a table in the EXCEPT clause of one publication but included by another"
);
$node_subscriber->safe_psql('postgres', 'DROP SUBSCRIPTION tap_sub');
$node_publisher->safe_psql('postgres', 'DROP PUBLICATION tap_pub1');
$node_publisher->safe_psql('postgres', 'DROP PUBLICATION tap_pub2');
$node_publisher->stop('fast');
done_testing();