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

On Thu, Jul 30, 2026 at 6:15 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Sun, May 31, 2026 at 11:30 PM Ajin Cherian <itsajin(at)gmail(dot)com> 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:

Thank you for updating the patch.

> 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, meaning that for a
FOR ALL TABLES publication it opened all tables, even before the fix.
On the other hand, check_publications_origin_tables() doesn't do so. I
think we should carefully consider that acquiring AccessShareLock on
all tables could introduce measurable overheads in execution time as
well as relcache bloat. A plain NULL check might work better here like
the v1 patch does. Even with the v1 patch, we could end up getting a
mismatched schema name and table name due to concurrent schema name
change or changing schema of the table. But the result would not be
serious; it just ends up adding a filter that could exclude unexpected
tables and missing some WARNINGs.

> 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.

> 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.

Regards,

--
Masahiko Sawada

Amazon Web Services: https://aws.amazon.com