| From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
|---|---|
| To: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
| Cc: | Ajin Cherian <itsajin(at)gmail(dot)com>, SATYANARAYANA NARLAPURAM <satyanarlapuram(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: [PATCH] Fix NULL dereference in subscription REFRESH on concurrent DROP |
| Date: | 2026-08-20 01:32:00 |
| Message-ID: | CALj2ACXoJG5EVeHtG8FHLkyc0zJKC7T3Ttqz+BOs_io7SyUnvw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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.
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-Fix-crash-in-subscription-REFRESH-on-concurrent-r.patch | application/octet-stream | 6.8 KB |
| nocfbot-v3-0001-PG19-Fix-crash-in-subscription-REFRESH-on-concurrent-r.patch | application/octet-stream | 3.4 KB |
| nocfbot-v3-0001-PG18-PG17-PG16-Fix-crash-in-subscription-REFRESH-on-concurrent-r.patch | application/octet-stream | 2.4 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Bingshuai Li | 2026-08-20 01:40:44 | Re: Logical Replication - revisit `is_table_publication` function implementation |
| Previous Message | David Rowley | 2026-08-20 01:07:01 | Re: Test tidscan,sql is not immune to autovacuum in v14 |