Re: [PATCH] Fix NULL dereference in subscription REFRESH on concurrent DROP

On Wed, Aug 19, 2026 at 6:34 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Tue, Aug 18, 2026 at 11:25 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> 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