On Wed, Aug 19, 2026 at 6:34 PM Bharath Rupireddy
<[email protected]> wrote:
>
> Hi,
>
> On Tue, Aug 18, 2026 at 11:25 PM Masahiko Sawada <[email protected]>
> wrote:
> >
> > Thank you for updating the patch.
>
> Thanks for reviewing it.
>
> > > 1/ Uses try_table_open() for all relations and gets the namespace
> > > using RelationGetNamespace().
> >
> > I'm not sure that this patch should follow the changes commit
> > 63e7a0d2c did for pg_get_publication_tables() since that function
> > opened tables if the column list is not specified
> >
> > On the other hand, check_publications_origin_tables() doesn't do so.
> >
> > A plain NULL check might work better here like
> > the v1 patch does.
>
> Agreed. I complicated this a bit with the additional table_open().
>
> > > 2/ Deduplicates the common code that quotes the subscription
> > > relations' schema-qualified names for tables and sequences into a
> > > helper function.
> >
> > While it works for v19 and master, it doesn't work for v16, v17, and
> > v18. ISTM it doesn't help simplify the code that much. I guess we can
> > live with such a duplication.
>
> Agreed.
>
> > > 3/ Adds a TAP test with an injection point in 0002 (which I don't
> > > intend to be committed).
> >
> > Thank you for creating the test case. I agree that the test for this
> > issue needs a new injection point and it would not be good for back
> > branches. I still see value a bit in having the regression test only
> > for HEAD. Feedback is welcome.
>
> As this is a crash fix, having a test makes sense. +1 for HEAD only. I
> tried without an injection point, but there's no table_open() or lock
> in this area to hold control and concurrently drop in another session.
> I moved the TAP test to the existing subscriber TAP test file to avoid
> starting another full-blown server just for this. Test now covers both
> concurrent table and sequence drop.
>
> Please find the attached v3 patch. I verified the issue goes back to
> PG16 where commit 8756930190 introduced the origin check. Patches for
> all affected branches are attached.
Thank you for updating the patch! Here are some review comments:
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -605,6 +605,69 @@ $result = $node_subscriber->safe_psql('postgres',
"SELECT count(*) FROM pg_replication_origin");
is($result, qq(0), 'check replication origin was dropped on subscriber');
+# Test that ALTER SUBSCRIPTION ... REFRESH PUBLICATION skips a subscribed
+# relation that is dropped concurrently during the refresh.
100_bugs.pl seems a better place to have have this regression test.
---
+ $bg->quit;
+
+ is($node_subscriber->safe_psql('postgres', 'SELECT 1;'),
+ '1', 'refresh survived a concurrently dropped table
and sequence');
While the background psql session survives with the proposed fix, the
ALTER SUBSCRIPTION ... REFRESH command still fails since the table and
sequence no longer exist on the subscriber. I think it's better to
make the command done without an error.
---
I think we can have the regression test in v19 too as it can be added easily.
I've made some changes including the above comments and updated the
commit message. I've attached the patch for HEAD. Please review it.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
From 167683841c8ca9a429c4996e0c918893758087f2 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
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 <[email protected]>
Author: SATYANARAYANA NARLAPURAM <[email protected]>
Co-authored-by: Bharath Rupireddy <[email protected]>
Reviewed-by: Ajin Cherian <[email protected]>
Reviewed-by: Masahiko Sawada <[email protected]>
Discussion: https://postgr.es/m/cahg+qdcd_o3707ey8c8b7hke-t14g8c0tk8me3ctywdsh3u...@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
