From 167683841c8ca9a429c4996e0c918893758087f2 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Wed, 19 Aug 2026 19:46:14 +0000 Subject: [PATCH v4] Fix crash in subscription refresh on concurrent relation drop. Commit 46b4f5c11b0 made the logical replication origin checks quote the schema and relation names they interpolate into the query sent to the publisher. Those names can be NULL, which that commit overlooked. AlterSubscription_refresh() collects the OIDs of the relations already present in pg_subscription_rel and hands them to check_publications_origin_tables() and check_publications_origin_sequences(), which append the schema-qualified name of each one to the query so that already-subscribed relations are excluded from the check. The relations are never locked, so one of them can be dropped concurrently before its name is read, and get_rel_name() and get_namespace_name() return NULL. quote_literal_cstr() dereferences it and crashes the backend. This commit fixes this by skipping a relation whose name is no longer available. A dropped relation is not synchronized anyway, and the appended clauses only exclude relations from a check whose sole effect is a WARNING, so omitting one can at most produce a spurious WARNING. The window is reachable from ALTER SUBSCRIPTION ... REFRESH PUBLICATION and from SET, ADD and DROP PUBLICATION, which refresh by default, but only when copy_data is true and origin is none. Backpatch to v16 as commit 46b4f5c11b0 was back-patched that far. The sequence path exists only in v19 and later. The test is applied to v19 and later only. Adding it to v17 and v18 would require enabling injection point support in src/test/subscription there, and v16 predates injection points entirely. That is more test infrastructure churn on stable branches than this fix warrants. Reported-by: SATYANARAYANA NARLAPURAM Author: SATYANARAYANA NARLAPURAM Co-authored-by: Bharath Rupireddy Reviewed-by: Ajin Cherian Reviewed-by: Masahiko Sawada Discussion: https://postgr.es/m/CAHg+QDcd_o3707Ey8c8b7HkE-t14g8c0tk8ME3ctywDsh3ut8g@mail.gmail.com Backpatch-through: 16 --- src/backend/commands/subscriptioncmds.c | 44 +++++++++--- src/test/subscription/t/100_bugs.pl | 89 +++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 8 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index e3c033339b5..22a61dca65d 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" @@ -1166,6 +1167,9 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data, subrel_states = GetSubscriptionRelations(sub->oid, true, true, false); subrel_count = list_length(subrel_states); + /* Allow a test to drop a subscribed relation before the origin check. */ + 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 @@ -3188,10 +3192,22 @@ check_publications_origin_tables(WalReceiverConn *wrconn, List *publications, 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); + char *schemaname; + char *tablename; + char *schemaname_lit; + char *tablename_lit; + + /* The table may have been dropped concurrently; skip if gone. */ + tablename = get_rel_name(relid); + if (tablename == NULL) + continue; + + schemaname = get_namespace_name(get_rel_namespace(relid)); + if (schemaname == NULL) + continue; + + schemaname_lit = quote_literal_cstr(schemaname); + tablename_lit = quote_literal_cstr(tablename); appendStringInfo(&cmd, "AND NOT (N.nspname = %s AND C.relname = %s)\n", schemaname_lit, tablename_lit); @@ -3315,10 +3331,22 @@ check_publications_origin_sequences(WalReceiverConn *wrconn, List *publications, 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); + char *schemaname; + char *seqname; + char *schemaname_lit; + char *seqname_lit; + + /* The sequence may have been dropped concurrently; skip if gone. */ + seqname = get_rel_name(relid); + if (seqname == NULL) + continue; + + schemaname = get_namespace_name(get_rel_namespace(relid)); + if (schemaname == NULL) + continue; + + schemaname_lit = quote_literal_cstr(schemaname); + seqname_lit = quote_literal_cstr(seqname); appendStringInfo(&cmd, "AND NOT (N.nspname = %s AND C.relname = %s)\n", diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index 335efd86bca..36a883f1fb1 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -673,4 +673,93 @@ $node_publisher->safe_psql('postgres', "DROP TABLE tab_upsert"); $node_publisher->stop('fast'); +$node_publisher->start; +$node_subscriber->start; + +SKIP: +{ + skip "injection points not supported by this build", 1 + if $node_subscriber->check_extension('injection_points') == 0; + + # Test that ALTER SUBSCRIPTION ... REFRESH PUBLICATION skips a subscribed + # relation that is dropped concurrently during the refresh. + + $node_publisher->rotate_logfile(); + $node_subscriber->rotate_logfile(); + + # Subscribe to a table and a sequence. + $node_publisher->safe_psql( + 'postgres', qq{ +CREATE TABLE tab_drop_refresh (a int); +CREATE SEQUENCE seq_drop_refresh; +CREATE PUBLICATION pub_drop_refresh FOR TABLE tab_drop_refresh; +CREATE PUBLICATION pub_seq_drop_refresh FOR ALL SEQUENCES; + }); + + $publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; + $node_subscriber->safe_psql( + 'postgres', qq{ +CREATE EXTENSION IF NOT EXISTS injection_points; +CREATE TABLE tab_drop_refresh (a int); +CREATE SEQUENCE seq_drop_refresh; +CREATE SUBSCRIPTION sub_drop_refresh + CONNECTION '$publisher_connstr' + PUBLICATION pub_drop_refresh, pub_seq_drop_refresh + WITH (copy_data = false, origin = none); + }); + $node_subscriber->wait_for_subscription_sync($node_publisher, + 'sub_drop_refresh'); + + $node_publisher->safe_psql( + 'postgres', + qq{ +ALTER PUBLICATION pub_drop_refresh DROP TABLE tab_drop_refresh; +DROP SEQUENCE seq_drop_refresh; + }); + + # Pause the refresh after it collects the relation list, drop the table + # and the sequence, then wake it. + $node_subscriber->safe_psql('postgres', + qq{SELECT injection_points_attach('subscription-refresh-before-origin-check', 'wait');} + ); + + my $psql = $node_subscriber->background_psql('postgres'); + $psql->query_until( + qr/starting_refresh/, q{ + \echo starting_refresh + ALTER SUBSCRIPTION sub_drop_refresh REFRESH PUBLICATION; + }); + + $node_subscriber->wait_for_event('client backend', + 'subscription-refresh-before-origin-check'); + + $node_subscriber->safe_psql( + 'postgres', qq{ +DROP TABLE tab_drop_refresh; +DROP SEQUENCE seq_drop_refresh; + }); + + $node_subscriber->safe_psql('postgres', + qq{SELECT injection_points_wakeup('subscription-refresh-before-origin-check');} + ); + + # quit() returns false unless psql exited cleanly, which it does not if the + # refresh errored out or the backend crashed. + ok($psql->quit, 'refresh completed without crashing the server'); + + $node_subscriber->safe_psql( + 'postgres', qq{ +SELECT injection_points_detach('subscription-refresh-before-origin-check'); +DROP SUBSCRIPTION sub_drop_refresh; + }); + $node_publisher->safe_psql('postgres', + qq{DROP PUBLICATION pub_drop_refresh, pub_seq_drop_refresh;}); + + $node_subscriber->stop('fast'); + $node_publisher->stop('fast'); +} + +$node_publisher->stop('fast'); +$node_subscriber->stop('fast'); + done_testing(); -- 2.55.0