From e658489aebded33fd18249aa303ef0cbbbc8d422 Mon Sep 17 00:00:00 2001 From: Ajin Cherian Date: Fri, 13 Nov 2026 14:37:19 +1100 Subject: [PATCH v3] Fix cache lookup failure in pg_get_publication_tables() after concurrent DROP PUBLICATION pg_get_publication_tables() collects (pubid, relid) pairs on its first call and then looks each publication up again per row with GetPublication(). No lock is held on the publication, so if it is dropped concurrently -- for instance while the function waits for the lock in try_table_open(), which also processes invalidations -- the lookup fails with an internal "cache lookup failed for publication" error. Reported-by: Noah Author: Ajin Cherian Reviewed-by: shveta malik Discussion: https://postgr.es/m/19864-5260a1fbeec7728c@postgresql.org --- src/backend/catalog/pg_publication.c | 72 +++++++++++++++++-- .../expected/pub-concurrent-drop.out | 24 +++++++ .../isolation/specs/pub-concurrent-drop.spec | 21 +++++- 3 files changed, 111 insertions(+), 6 deletions(-) diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index f11ea8839ca..05cca134065 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -1305,9 +1305,12 @@ GetAllSchemaPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt) * Get publication using oid * * The Publication struct and its data are palloc'ed here. + * + * If missing_ok is true, return NULL when the publication does not exist + * (e.g., it was dropped concurrently); otherwise raise an error. */ -Publication * -GetPublication(Oid pubid) +static Publication * +get_publication_internal(Oid pubid, bool missing_ok) { HeapTuple tup; Publication *pub; @@ -1315,7 +1318,11 @@ GetPublication(Oid pubid) tup = SearchSysCache1(PUBLICATIONOID, ObjectIdGetDatum(pubid)); if (!HeapTupleIsValid(tup)) + { + if (missing_ok) + return NULL; elog(ERROR, "cache lookup failed for publication %u", pubid); + } pubform = (Form_pg_publication) GETSTRUCT(tup); @@ -1336,6 +1343,12 @@ GetPublication(Oid pubid) return pub; } +Publication * +GetPublication(Oid pubid) +{ + return get_publication_internal(pubid, false); +} + /* * Get Publication using name. */ @@ -1444,6 +1457,27 @@ is_table_publishable_in_publication(Oid relid, Publication *pub) ObjectIdGetDatum(pub->oid))); } +/* + * A helper function for pg_get_publication_tables() to get the name of the + * publication with the given OID from the list of publications collected on + * the first call. It is used to report a publication that has been dropped + * concurrently, whose name can no longer be looked up in the catalog. + */ +static char * +get_pubname_from_list(List *pubs, Oid pubid) +{ + foreach_ptr(Publication, pub, pubs) + { + if (pub->oid == pubid) + return pub->name; + } + + /* The publication must have been collected on the first call. */ + elog(ERROR, "could not find publication %u in the list of publications", + pubid); + return NULL; /* keep compiler quiet */ +} + /* * Helper function to get information of the tables in the given * publication(s). @@ -1471,6 +1505,7 @@ pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames, typedef struct { List *table_infos; /* list of published_rel */ + List *pubs; /* list of Publication, for error messages */ int curr_idx; /* current index into table_infos */ } publication_tables_state; @@ -1483,6 +1518,7 @@ pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames, TupleDesc tupdesc; MemoryContext oldcontext; List *table_infos = NIL; + List *pubs = NIL; Datum *elems; int nelems, i; @@ -1521,6 +1557,8 @@ pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames, if (pub_elem == NULL) continue; + pubs = lappend(pubs, pub_elem); + if (filter_by_relid) { /* Check if the given table is published for the publication */ @@ -1609,6 +1647,7 @@ pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames, /* Store the state to be used across SRF calls. */ ptstate = palloc_object(publication_tables_state); ptstate->table_infos = table_infos; + ptstate->pubs = pubs; ptstate->curr_idx = 0; funcctx->user_fctx = ptstate; @@ -1647,10 +1686,35 @@ pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames, continue; /* - * Form tuple with appropriate data. + * The publication OIDs were also collected earlier and we don't hold + * a lock on the publication, so it may have been dropped concurrently + * (for instance, while we waited for the lock on the table above, + * which also processes pending invalidations). + * + * Unlike a dropped table, we can't just skip it: rows for this + * publication may already have been returned, so the result would be + * a misleading partial list. Report it as an error instead, unless + * the caller asked for missing publications to be ignored, which is + * consistent with how they are treated on the first call. */ + pub = get_publication_internal(table_info->pubid, true); + if (pub == NULL) + { + if (!pub_missing_ok) + ereport(ERROR, + errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("publication \"%s\" does not exist", + get_pubname_from_list(ptstate->pubs, + table_info->pubid)), + errdetail("The publication was dropped concurrently.")); + + table_close(rel, AccessShareLock); + continue; + } - pub = GetPublication(table_info->pubid); + /* + * Form tuple with appropriate data. + */ values[0] = ObjectIdGetDatum(pub->oid); values[1] = ObjectIdGetDatum(relid); diff --git a/src/test/isolation/expected/pub-concurrent-drop.out b/src/test/isolation/expected/pub-concurrent-drop.out index 8360af0ec9c..e31d06f917d 100644 --- a/src/test/isolation/expected/pub-concurrent-drop.out +++ b/src/test/isolation/expected/pub-concurrent-drop.out @@ -14,3 +14,27 @@ tablename pubdrop.keepme (1 row) + +starting permutation: lock list_pub_tables drop_pub_and_commit +step lock: BEGIN; LOCK pubdrop.dropme IN ACCESS EXCLUSIVE MODE; +step list_pub_tables: + SELECT relid::regclass AS tablename + FROM pg_get_publication_tables('pub_schema') + ORDER BY tablename; + +step drop_pub_and_commit: DROP PUBLICATION pub_schema; COMMIT; +step list_pub_tables: <... completed> +ERROR: publication "pub_schema" does not exist + +starting permutation: lock list_pub_table drop_pub_and_commit +step lock: BEGIN; LOCK pubdrop.dropme IN ACCESS EXCLUSIVE MODE; +step list_pub_table: + SELECT relid::regclass AS tablename + FROM pg_get_publication_tables(ARRAY['pub_schema'], 'pubdrop.dropme'::regclass); + +step drop_pub_and_commit: DROP PUBLICATION pub_schema; COMMIT; +step list_pub_table: <... completed> +tablename +--------- +(0 rows) + diff --git a/src/test/isolation/specs/pub-concurrent-drop.spec b/src/test/isolation/specs/pub-concurrent-drop.spec index 4f7d701d60c..3b41ced1ba5 100644 --- a/src/test/isolation/specs/pub-concurrent-drop.spec +++ b/src/test/isolation/specs/pub-concurrent-drop.spec @@ -1,4 +1,4 @@ -# Tests for concurrently dropping a relation while a publication's tables are +# Tests for concurrently dropping a relation or the publication itself while a publication's tables are # being listed. setup @@ -12,12 +12,13 @@ setup teardown { DROP SCHEMA pubdrop CASCADE; - DROP PUBLICATION pub_schema; + DROP PUBLICATION IF EXISTS pub_schema; } session s1 step lock { BEGIN; LOCK pubdrop.dropme IN ACCESS EXCLUSIVE MODE; } step drop_and_commit { DROP TABLE pubdrop.dropme; COMMIT; } +step drop_pub_and_commit { DROP PUBLICATION pub_schema; COMMIT; } session s2 step list_pub_tables @@ -26,6 +27,11 @@ step list_pub_tables FROM pg_get_publication_tables('pub_schema') ORDER BY tablename; } +step list_pub_table +{ + SELECT relid::regclass AS tablename + FROM pg_get_publication_tables(ARRAY['pub_schema'], 'pubdrop.dropme'::regclass); +} # Hold an ACCESS EXCLUSIVE lock on the table in one session, so that the query # listing a publication's tables in another session blocks when it tries to @@ -34,3 +40,14 @@ step list_pub_tables # skips the now-dropped table instead of erroring with "could not open relation # with OID". permutation lock list_pub_tables drop_and_commit + +# Same as above, but drop the publication instead of the table. When the +# query resumes, the publication it collected the table OIDs from is gone. +# Rather than returning a partial list, it reports that the publication was +# concurrently dropped instead of failing with "cache lookup failed for +# publication". +permutation lock list_pub_tables drop_pub_and_commit + +# The variant that takes a table OID ignores missing publications, so it +# skips the row for the dropped publication instead. +permutation lock list_pub_table drop_pub_and_commit -- 2.47.3