Hi, On Sun, May 31, 2026 at 11:30 PM Ajin Cherian <[email protected]> wrote: > > I've reviewed the patch and it looks good
Thanks Satya for the off-list discussion, and thanks Ajin for the review. > just a small comment - instead of silently continuing after finding a dropped > local table, would you want to log an error message? IMHO, logging a message is unnecessary. The origin check only raises a WARNING and doesn't drive the actual refresh, so silently skipping a concurrently dropped relation is harmless. A similar fix for concurrent relation drops in pg_get_publication_tables() is in commit 63e7a0d2c, and I'm following it in using try_table_open() and in not logging for dropped local relations. Please find attached the v2 patch. It has the following changes: 1/ Uses try_table_open() for all relations and gets the namespace using RelationGetNamespace(). 2/ Deduplicates the common code that quotes the subscription relations' schema-qualified names for tables and sequences into a helper function. 3/ Adds a TAP test with an injection point in 0002 (which I don't intend to be committed). We may need to backpatch the tables and sequences fix down to PG19, and just the tables fix down to PG16 (commit 8756930190). Thoughts? -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
From 662fd2e4065ff70f5a0139e63ae1fa954c7c9389 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Fri, 31 Jul 2026 00:24:57 +0000 Subject: [PATCH v2 v2 1/2] Fix crash in subscription REFRESH on concurrent relation drop. check_publications_origin_tables() and check_publications_origin_sequences() build a query that excludes the subscription's local relations by name, so that already-subscribed relations are not re-synchronized. They walk the local relation OIDs without holding any lock and look up each relation's schema and name. If a relation is dropped concurrently, those lookups return InvalidOid and NULL, and the NULL name is passed to quote_literal_cstr(), which dereferences it unconditionally and crashes the backend. This can happen during ALTER SUBSCRIPTION ... REFRESH PUBLICATION when a subscribed table or sequence is dropped by another session. Fix by opening each relation with try_table_open(), which returns NULL if it is already gone, and skipping it in that case, as a dropped relation is not synchronized anyway. The schema and name are then read from the open relation instead of from unlocked syscache lookups. The two functions carried an identical copy of this loop, so it is factored out into a new helper, append_quoted_rel_filters(). The table path may need to be backpatched down to 16 (commit 8756930190). Author: Satyanarayana Narlapuram <[email protected]> Co-authored-by: Bharath Rupireddy <[email protected]> Reported-by: Satyanarayana Narlapuram <[email protected]> Reviewed-by: Ajin Cherian <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAHg%2BQDcd_o3707Ey8c8b7HkE-t14g8c0tk8ME3ctywDsh3ut8g%40mail.gmail.com --- src/backend/commands/subscriptioncmds.c | 74 ++++++++++++++----------- 1 file changed, 42 insertions(+), 32 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 013ac46db07..05d46fe05f3 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -127,6 +127,8 @@ typedef struct PublicationRelKind } PublicationRelKind; static List *fetch_relation_list(WalReceiverConn *wrconn, List *publications); +static void append_quoted_rel_filters(StringInfo cmd, Oid *relids, + int nrelids); static void check_publications_origin_tables(WalReceiverConn *wrconn, List *publications, bool copydata, bool retain_dead_tuples, @@ -3069,6 +3071,44 @@ AlterSubscriptionOwner_oid(Oid subid, Oid newOwnerId) table_close(rel, RowExclusiveLock); } +/* + * Append filter clauses to the origin-check query that exclude the given local + * relations, matching each by its schema-qualified name. + * + * The local relations are not locked, so a relation may be dropped + * concurrently. try_table_open() returns NULL if it is already gone, in which + * case it is skipped, as a dropped relation is not synchronized. + */ +static void +append_quoted_rel_filters(StringInfo cmd, Oid *relids, int nrelids) +{ + for (int i = 0; i < nrelids; i++) + { + Relation rel; + char *schemaname; + char *relname; + char *schemaname_lit; + char *relname_lit; + + rel = try_table_open(relids[i], AccessShareLock); + if (rel == NULL) + continue; + + schemaname = get_namespace_name(RelationGetNamespace(rel)); + relname = RelationGetRelationName(rel); + schemaname_lit = quote_literal_cstr(schemaname); + relname_lit = quote_literal_cstr(relname); + + appendStringInfo(cmd, "AND NOT (N.nspname = %s AND C.relname = %s)\n", + schemaname_lit, relname_lit); + + pfree(schemaname_lit); + pfree(relname_lit); + + table_close(rel, AccessShareLock); + } +} + /* * Check and log a warning if the publisher has subscribed to the same table, * its partition ancestors (if it's a partition), or its partition children (if @@ -3103,7 +3143,6 @@ check_publications_origin_tables(WalReceiverConn *wrconn, List *publications, TupleTableSlot *slot; Oid tableRow[1] = {TEXTOID}; List *publist = NIL; - int i; bool check_rdt; bool check_table_sync; bool origin_none = origin && @@ -3152,22 +3191,7 @@ check_publications_origin_tables(WalReceiverConn *wrconn, List *publications, * created subscriptions on the publisher. */ if (check_table_sync) - { - for (i = 0; i < subrel_count; i++) - { - Oid relid = subrel_local_oids[i]; - char *schemaname = get_namespace_name(get_rel_namespace(relid)); - char *tablename = get_rel_name(relid); - char *schemaname_lit = quote_literal_cstr(schemaname); - char *tablename_lit = quote_literal_cstr(tablename); - - appendStringInfo(&cmd, "AND NOT (N.nspname = %s AND C.relname = %s)\n", - schemaname_lit, tablename_lit); - - pfree(schemaname_lit); - pfree(tablename_lit); - } - } + append_quoted_rel_filters(&cmd, subrel_local_oids, subrel_count); res = walrcv_exec(wrconn, cmd.data, 1, tableRow); pfree(cmd.data); @@ -3280,21 +3304,7 @@ check_publications_origin_sequences(WalReceiverConn *wrconn, List *publications, * present on the subscriber. This check should be skipped as these will * not be re-synced. */ - for (int i = 0; i < subrel_count; i++) - { - Oid relid = subrel_local_oids[i]; - char *schemaname = get_namespace_name(get_rel_namespace(relid)); - char *seqname = get_rel_name(relid); - char *schemaname_lit = quote_literal_cstr(schemaname); - char *seqname_lit = quote_literal_cstr(seqname); - - appendStringInfo(&cmd, - "AND NOT (N.nspname = %s AND C.relname = %s)\n", - schemaname_lit, seqname_lit); - - pfree(schemaname_lit); - pfree(seqname_lit); - } + append_quoted_rel_filters(&cmd, subrel_local_oids, subrel_count); res = walrcv_exec(wrconn, cmd.data, 1, tableRow); pfree(cmd.data); -- 2.47.3
From f29b4fa3c7f47605dc6569a2528cdc4f3ed885b4 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Fri, 31 Jul 2026 00:26:13 +0000 Subject: [PATCH v2 v2 2/2] Test subscription REFRESH with concurrently dropped relation. Add a TAP test for the crash fixed in the previous commit. A new injection point pauses the refresh in the narrow window between collecting the subscription's local relation list and reading each relation's name. The test parks a background refresh there, drops a subscribed table, then wakes the refresh, which crashed before the fix and now skips the dropped relation. Author: Bharath Rupireddy <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAHg%2BQDcd_o3707Ey8c8b7HkE-t14g8c0tk8ME3ctywDsh3ut8g%40mail.gmail.com --- src/backend/commands/subscriptioncmds.c | 8 ++ src/test/subscription/meson.build | 1 + .../t/039_refresh_concurrent_drop.pl | 90 +++++++++++++++++++ 3 files changed, 99 insertions(+) create mode 100644 src/test/subscription/t/039_refresh_concurrent_drop.pl diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 05d46fe05f3..bd815a9ed5b 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -54,6 +54,7 @@ #include "utils/acl.h" #include "utils/builtins.h" #include "utils/guc.h" +#include "utils/injection_point.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/pg_lsn.h" @@ -1139,6 +1140,13 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data, subrel_states = GetSubscriptionRelations(sub->oid, true, true, false); subrel_count = list_length(subrel_states); + /* + * The local relation OIDs are now captured but not locked, so a + * concurrent drop can make them stale before the origin checks run. + * This injection point lets a test reproduce that window. + */ + INJECTION_POINT("subscription-refresh-before-origin-check", NULL); + /* * Build qsorted arrays of local table oids and sequence oids for * faster lookup. This can potentially contain all tables and diff --git a/src/test/subscription/meson.build b/src/test/subscription/meson.build index e71e95c6297..33b16f9ac0c 100644 --- a/src/test/subscription/meson.build +++ b/src/test/subscription/meson.build @@ -48,6 +48,7 @@ tests += { 't/036_sequences.pl', 't/037_except.pl', 't/038_walsnd_shutdown_timeout.pl', + 't/039_refresh_concurrent_drop.pl', 't/100_bugs.pl', ], }, diff --git a/src/test/subscription/t/039_refresh_concurrent_drop.pl b/src/test/subscription/t/039_refresh_concurrent_drop.pl new file mode 100644 index 00000000000..a01e2f6b355 --- /dev/null +++ b/src/test/subscription/t/039_refresh_concurrent_drop.pl @@ -0,0 +1,90 @@ +# Copyright (c) 2025-2026, PostgreSQL Global Development Group + +# Test that ALTER SUBSCRIPTION ... REFRESH PUBLICATION skips, rather than +# crashes on, a subscribed relation dropped concurrently during the refresh. +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node_publisher = PostgreSQL::Test::Cluster->new('publisher'); +$node_publisher->init(allows_streaming => 'logical'); +$node_publisher->start; + +my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber'); +$node_subscriber->init; +$node_subscriber->start; + +my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; + +# The refresh runs on the subscriber, so arm the injection point there. +$node_subscriber->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +# tab_keep stays, tab_drop is dropped during the refresh. +$node_publisher->safe_psql( + 'postgres', q{ + CREATE TABLE tab_keep (a int); + CREATE TABLE tab_drop (a int); + CREATE PUBLICATION mypub FOR ALL TABLES; +}); + +$node_subscriber->safe_psql( + 'postgres', q{ + CREATE TABLE tab_keep (a int); + CREATE TABLE tab_drop (a int); +}); + +# origin = none is the path that reads each local relation's name. +$node_subscriber->safe_psql( + 'postgres', qq{ + CREATE SUBSCRIPTION mysub CONNECTION '$publisher_connstr' + PUBLICATION mypub WITH (copy_data = false, origin = none); +}); + +$node_subscriber->wait_for_subscription_sync($node_publisher, 'mysub'); + +# Pause the next refresh after it collects the local relation list. +$node_subscriber->safe_psql('postgres', + "SELECT injection_points_attach('subscription-refresh-before-origin-check', 'wait');" +); + +# Run the refresh in the background. It blocks at the injection point. +my $bg = $node_subscriber->background_psql('postgres'); +$bg->query_until( + qr/starting_refresh/, q{ + \echo starting_refresh + ALTER SUBSCRIPTION mysub REFRESH PUBLICATION; +}); + +$node_subscriber->wait_for_event('client backend', + 'subscription-refresh-before-origin-check'); + +# Drop the table while the refresh holds its now-stale OID. +$node_subscriber->safe_psql('postgres', 'DROP TABLE tab_drop;'); + +# Wake the refresh. It crashed here before the fix. +$node_subscriber->safe_psql('postgres', + "SELECT injection_points_wakeup('subscription-refresh-before-origin-check');" +); + +$bg->quit; + +# A successful query proves the backend survived. +is($node_subscriber->safe_psql('postgres', 'SELECT 1;'), + '1', 'subscriber survived refresh with concurrently dropped table'); + +$node_subscriber->safe_psql('postgres', + "SELECT injection_points_detach('subscription-refresh-before-origin-check');" +); + +$node_subscriber->stop; +$node_publisher->stop; + +done_testing(); -- 2.47.3
