Horton

Thread

  1. Re: Fix race condition in pg_get_publication_tables with concurrent DROP TABLE

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2026-05-01T01:30:00Z

    Hi,

On Wed, Apr 29, 2026 at 8:41 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
> > <v5-0001-PG16-Fix-pg_get_publication_tables-race-with-conc.txt><v5-0001-Fix-pg_get_publication_tables-race-with-concurren.patch><v5-0001-PG18-Fix-pg_get_publication_tables-race-with-conc.txt><v5-0001-PG17-Fix-pg_get_publication_tables-race-with-conc.txt>
>
> I am afraid this is only a partial fix.

Thanks for reviewing it. Please find my responses below.

> ```
> @@ -1599,12 +1621,18 @@ pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames,
>                 /* Show all columns when the column list is not specified. */
>                 if (nulls[2])
>                 {
> -                       Relation        rel = table_open(relid, AccessShareLock);
> +                       Relation        rel = try_table_open(relid, AccessShareLock);
>                         int                     nattnums = 0;
>                         int16      *attnums;
> -                       TupleDesc       desc = RelationGetDescr(rel);
> +                       TupleDesc       desc;
>                         int                     i;
>
> +                       /* Skip if the relation has been concurrently dropped. */
> +                       if (rel == NULL)
> +                               continue;
> ```
>
> This change uses try_table_open() to detect whether a table has been dropped, but try_table_open() is only called when the column list is not specified. If a table is included in the publication with an explicit column list, then even if it is dropped concurrently, it may still be returned by pg_get_publication_tables().

Right. The try_table_open() is only needed there because that's the
only code path that actually opens the relation (to enumerate its
columns). The column list path reads from the pg_publication_rel
catalog - it never calls table_open(), so it cannot hit the ERROR.

> So this patch removes the “could not open relation with OID” error, but it does not fully ensure the accuracy of the returned table list.

IMO, no function returning table OIDs can guarantee they remain valid
- a drop can happen right after we return the OID, and accuracy is in
the caller's hands. All the callers of pg_get_publication_tables()
already handle this by JOINing with pg_class.

> It also introduces inconsistent behavior between tables published with and without column lists.

These two paths do different things - one needs the relation open, the
other doesn't. For callers, the outcome is the same: any stale OID
gets filtered out by their pg_class JOIN.

> To resolve the race condition completely, I think we should try to open the table regardless of whether a column list is specified.

IMO, that would add unnecessary locking overhead in a path that
doesn't need it. The bug is the ERROR, and this patch fixes it.

In short, when no column list is specified,
pg_get_publication_tables() needs to open the relation to enumerate
all publishable columns and return them as an int2vector in the attrs
output column. It currently uses table_open() for this, which errors
out with concurrent table drops. This patch fixes it by using
try_table_open() and skipping the relation if it's been dropped.

Thoughts?

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

  2. Re: Fix race condition in pg_get_publication_tables with concurrent DROP TABLE

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2026-05-04T02:34:00Z

    Hi,

On Wed, Apr 29, 2026 at 12:15 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Fixed. Please find the attached v5 patch.
>
> The fix is needed only for PG16 and later, not PG15 or PG14. The bug
> was introduced by b7ae03953690 [1] in PG16, which added a table_open()
> call in pg_get_publication_tables(). PG15 and earlier only use
> get_rel_namespace() and syscache lookups, both of which gracefully
> handle dropped relations (returning InvalidOid/false rather than
> erroring).
>
> I verified the bug and the fix on all affected branches. Please find
> the attached version-specific patches for backpatching. Thank you!
>
> [1] b7ae03953690 - Ignore dropped and generated columns from the column list

Please find the attached v6 patch, which fixes a test failure on
FreeBSD. This variant of the build forces parallel query via
debug_parallel_query=regress, causing the pg_get_publication_tables
injection point to fire in parallel workers as well. I disabled forced
parallel query for this test case.

Thank you!


--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

  3. Re: Fix race condition in pg_get_publication_tables with concurrent DROP TABLE

    shveta malik <shveta.malik@gmail.com> — 2026-05-04T05:08:10Z

    On Fri, May 1, 2026 at 7:00 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> On Wed, Apr 29, 2026 at 8:41 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >
> > > <v5-0001-PG16-Fix-pg_get_publication_tables-race-with-conc.txt><v5-0001-Fix-pg_get_publication_tables-race-with-concurren.patch><v5-0001-PG18-Fix-pg_get_publication_tables-race-with-conc.txt><v5-0001-PG17-Fix-pg_get_publication_tables-race-with-conc.txt>
> >
> > I am afraid this is only a partial fix.
>
> Thanks for reviewing it. Please find my responses below.
>
> > ```
> > @@ -1599,12 +1621,18 @@ pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames,
> >                 /* Show all columns when the column list is not specified. */
> >                 if (nulls[2])
> >                 {
> > -                       Relation        rel = table_open(relid, AccessShareLock);
> > +                       Relation        rel = try_table_open(relid, AccessShareLock);
> >                         int                     nattnums = 0;
> >                         int16      *attnums;
> > -                       TupleDesc       desc = RelationGetDescr(rel);
> > +                       TupleDesc       desc;
> >                         int                     i;
> >
> > +                       /* Skip if the relation has been concurrently dropped. */
> > +                       if (rel == NULL)
> > +                               continue;
> > ```
> >
> > This change uses try_table_open() to detect whether a table has been dropped, but try_table_open() is only called when the column list is not specified. If a table is included in the publication with an explicit column list, then even if it is dropped concurrently, it may still be returned by pg_get_publication_tables().
>
> Right. The try_table_open() is only needed there because that's the
> only code path that actually opens the relation (to enumerate its
> columns). The column list path reads from the pg_publication_rel
> catalog - it never calls table_open(), so it cannot hit the ERROR.
>
> > So this patch removes the “could not open relation with OID” error, but it does not fully ensure the accuracy of the returned table list.
>
> IMO, no function returning table OIDs can guarantee they remain valid
> - a drop can happen right after we return the OID, and accuracy is in
> the caller's hands. All the callers of pg_get_publication_tables()
> already handle this by JOINing with pg_class.
>

I agree with Bharath. Also I would like to add one more point. We do have this:

+ /* Skip if the relation has been concurrently dropped. */
+ if (!OidIsValid(schemaid))
+ continue;

So if a table is dropped before we could access its explicitly
mentioned column-list, above should handle it right even without
'try_table_open' done? Moreover, pg_publication_rel will not return
any row for dropped table, so we should be good. For 'table_infos',
the case was different, as we saved the list in first call and are
using that in subsequent call? But that is not the case with
pg_publication_rel.

> > It also introduces inconsistent behavior between tables published with and without column lists.
>
> These two paths do different things - one needs the relation open, the
> other doesn't. For callers, the outcome is the same: any stale OID
> gets filtered out by their pg_class JOIN.
>
> > To resolve the race condition completely, I think we should try to open the table regardless of whether a column list is specified.
>
> IMO, that would add unnecessary locking overhead in a path that
> doesn't need it. The bug is the ERROR, and this patch fixes it.
>
> In short, when no column list is specified,
> pg_get_publication_tables() needs to open the relation to enumerate
> all publishable columns and return them as an int2vector in the attrs
> output column. It currently uses table_open() for this, which errors
> out with concurrent table drops. This patch fixes it by using
> try_table_open() and skipping the relation if it's been dropped.
>
> Thoughts?
>
> --
> Bharath Rupireddy
> Amazon Web Services: https://aws.amazon.com

  4. Re: Fix race condition in pg_get_publication_tables with concurrent DROP TABLE

    Chao Li <li.evan.chao@gmail.com> — 2026-05-05T03:08:29Z

    
> On May 1, 2026, at 09:30, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> 
> Hi,
> 
> On Wed, Apr 29, 2026 at 8:41 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> 
>>> <v5-0001-PG16-Fix-pg_get_publication_tables-race-with-conc.txt><v5-0001-Fix-pg_get_publication_tables-race-with-concurren.patch><v5-0001-PG18-Fix-pg_get_publication_tables-race-with-conc.txt><v5-0001-PG17-Fix-pg_get_publication_tables-race-with-conc.txt>
>> 
>> I am afraid this is only a partial fix.
> 
> Thanks for reviewing it. Please find my responses below.
> 
>> ```
>> @@ -1599,12 +1621,18 @@ pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames,
>>                /* Show all columns when the column list is not specified. */
>>                if (nulls[2])
>>                {
>> -                       Relation        rel = table_open(relid, AccessShareLock);
>> +                       Relation        rel = try_table_open(relid, AccessShareLock);
>>                        int                     nattnums = 0;
>>                        int16      *attnums;
>> -                       TupleDesc       desc = RelationGetDescr(rel);
>> +                       TupleDesc       desc;
>>                        int                     i;
>> 
>> +                       /* Skip if the relation has been concurrently dropped. */
>> +                       if (rel == NULL)
>> +                               continue;
>> ```
>> 
>> This change uses try_table_open() to detect whether a table has been dropped, but try_table_open() is only called when the column list is not specified. If a table is included in the publication with an explicit column list, then even if it is dropped concurrently, it may still be returned by pg_get_publication_tables().
> 
> Right. The try_table_open() is only needed there because that's the
> only code path that actually opens the relation (to enumerate its
> columns). The column list path reads from the pg_publication_rel
> catalog - it never calls table_open(), so it cannot hit the ERROR.
> 
>> So this patch removes the “could not open relation with OID” error, but it does not fully ensure the accuracy of the returned table list.
> 
> IMO, no function returning table OIDs can guarantee they remain valid
> - a drop can happen right after we return the OID, and accuracy is in
> the caller's hands. All the callers of pg_get_publication_tables()
> already handle this by JOINing with pg_class.
> 

Fair.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  5. Re: Fix race condition in pg_get_publication_tables with concurrent DROP TABLE

    Chao Li <li.evan.chao@gmail.com> — 2026-05-05T03:10:10Z

    
> On May 4, 2026, at 13:08, shveta malik <shveta.malik@gmail.com> wrote:
> 
> On Fri, May 1, 2026 at 7:00 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>> 
>> Hi,
>> 
>> On Wed, Apr 29, 2026 at 8:41 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>> 
>>>> <v5-0001-PG16-Fix-pg_get_publication_tables-race-with-conc.txt><v5-0001-Fix-pg_get_publication_tables-race-with-concurren.patch><v5-0001-PG18-Fix-pg_get_publication_tables-race-with-conc.txt><v5-0001-PG17-Fix-pg_get_publication_tables-race-with-conc.txt>
>>> 
>>> I am afraid this is only a partial fix.
>> 
>> Thanks for reviewing it. Please find my responses below.
>> 
>>> ```
>>> @@ -1599,12 +1621,18 @@ pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames,
>>>                /* Show all columns when the column list is not specified. */
>>>                if (nulls[2])
>>>                {
>>> -                       Relation        rel = table_open(relid, AccessShareLock);
>>> +                       Relation        rel = try_table_open(relid, AccessShareLock);
>>>                        int                     nattnums = 0;
>>>                        int16      *attnums;
>>> -                       TupleDesc       desc = RelationGetDescr(rel);
>>> +                       TupleDesc       desc;
>>>                        int                     i;
>>> 
>>> +                       /* Skip if the relation has been concurrently dropped. */
>>> +                       if (rel == NULL)
>>> +                               continue;
>>> ```
>>> 
>>> This change uses try_table_open() to detect whether a table has been dropped, but try_table_open() is only called when the column list is not specified. If a table is included in the publication with an explicit column list, then even if it is dropped concurrently, it may still be returned by pg_get_publication_tables().
>> 
>> Right. The try_table_open() is only needed there because that's the
>> only code path that actually opens the relation (to enumerate its
>> columns). The column list path reads from the pg_publication_rel
>> catalog - it never calls table_open(), so it cannot hit the ERROR.
>> 
>>> So this patch removes the “could not open relation with OID” error, but it does not fully ensure the accuracy of the returned table list.
>> 
>> IMO, no function returning table OIDs can guarantee they remain valid
>> - a drop can happen right after we return the OID, and accuracy is in
>> the caller's hands. All the callers of pg_get_publication_tables()
>> already handle this by JOINing with pg_class.
>> 
> 
> I agree with Bharath. Also I would like to add one more point. We do have this:
> 
> + /* Skip if the relation has been concurrently dropped. */
> + if (!OidIsValid(schemaid))
> + continue;
> 

Actually, this is the other comment I have. Why the comment says “if the relation has been dropped”, but the actual check is on schema id?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  6. Re: Fix race condition in pg_get_publication_tables with concurrent DROP TABLE

    shveta malik <shveta.malik@gmail.com> — 2026-05-05T04:02:00Z

    On Tue, May 5, 2026 at 8:40 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On May 4, 2026, at 13:08, shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, May 1, 2026 at 7:00 AM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >> On Wed, Apr 29, 2026 at 8:41 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >>>
> >>>> <v5-0001-PG16-Fix-pg_get_publication_tables-race-with-conc.txt><v5-0001-Fix-pg_get_publication_tables-race-with-concurren.patch><v5-0001-PG18-Fix-pg_get_publication_tables-race-with-conc.txt><v5-0001-PG17-Fix-pg_get_publication_tables-race-with-conc.txt>
> >>>
> >>> I am afraid this is only a partial fix.
> >>
> >> Thanks for reviewing it. Please find my responses below.
> >>
> >>> ```
> >>> @@ -1599,12 +1621,18 @@ pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames,
> >>>                /* Show all columns when the column list is not specified. */
> >>>                if (nulls[2])
> >>>                {
> >>> -                       Relation        rel = table_open(relid, AccessShareLock);
> >>> +                       Relation        rel = try_table_open(relid, AccessShareLock);
> >>>                        int                     nattnums = 0;
> >>>                        int16      *attnums;
> >>> -                       TupleDesc       desc = RelationGetDescr(rel);
> >>> +                       TupleDesc       desc;
> >>>                        int                     i;
> >>>
> >>> +                       /* Skip if the relation has been concurrently dropped. */
> >>> +                       if (rel == NULL)
> >>> +                               continue;
> >>> ```
> >>>
> >>> This change uses try_table_open() to detect whether a table has been dropped, but try_table_open() is only called when the column list is not specified. If a table is included in the publication with an explicit column list, then even if it is dropped concurrently, it may still be returned by pg_get_publication_tables().
> >>
> >> Right. The try_table_open() is only needed there because that's the
> >> only code path that actually opens the relation (to enumerate its
> >> columns). The column list path reads from the pg_publication_rel
> >> catalog - it never calls table_open(), so it cannot hit the ERROR.
> >>
> >>> So this patch removes the “could not open relation with OID” error, but it does not fully ensure the accuracy of the returned table list.
> >>
> >> IMO, no function returning table OIDs can guarantee they remain valid
> >> - a drop can happen right after we return the OID, and accuracy is in
> >> the caller's hands. All the callers of pg_get_publication_tables()
> >> already handle this by JOINing with pg_class.
> >>
> >
> > I agree with Bharath. Also I would like to add one more point. We do have this:
> >
> > + /* Skip if the relation has been concurrently dropped. */
> > + if (!OidIsValid(schemaid))
> > + continue;
> >
>
> Actually, this is the other comment I have. Why the comment says “if the relation has been dropped”, but the actual check is on schema id?
>

Okay, I see your point. Shall we tweak it to the below to make it more
understandable?


Oid schemaid;

/*
* get_rel_namespace() returns InvalidOid if the relation no longer exists
* (e.g., dropped concurrently). Skip such entries.
*/
if (!OidIsValid(schemaid = get_rel_namespace(relid)))
continue;

thanks
Shveta

  7. Re: Fix race condition in pg_get_publication_tables with concurrent DROP TABLE

    Masahiko Sawada <sawada.mshk@gmail.com> — 2026-05-12T21:20:14Z

    Hi,

On Sun, May 3, 2026 at 7:35 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> On Wed, Apr 29, 2026 at 12:15 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > Fixed. Please find the attached v5 patch.
> >
> > The fix is needed only for PG16 and later, not PG15 or PG14. The bug
> > was introduced by b7ae03953690 [1] in PG16, which added a table_open()
> > call in pg_get_publication_tables(). PG15 and earlier only use
> > get_rel_namespace() and syscache lookups, both of which gracefully
> > handle dropped relations (returning InvalidOid/false rather than
> > erroring).
> >
> > I verified the bug and the fix on all affected branches. Please find
> > the attached version-specific patches for backpatching. Thank you!
> >
> > [1] b7ae03953690 - Ignore dropped and generated columns from the column list
>
> Please find the attached v6 patch, which fixes a test failure on
> FreeBSD. This variant of the build forces parallel query via
> debug_parallel_query=regress, causing the pg_get_publication_tables
> injection point to fire in parallel workers as well. I disabled forced
> parallel query for this test case.
>

I reviewed the patch and here are some comments:

+/* State for pg_get_publication_tables SRF */
+typedef struct
+{
+ List    *table_infos; /* list of published_rel */
+ int curr_idx; /* current index into table_infos */
+} publication_tables_state;

I think we can define publication_table_state in
pg_get_publication_tables() as it's used only in that function.

---
+ /* Skip if the relation has been concurrently dropped. */
+ if (!OidIsValid(schemaid))
+ continue;

Although this check is done for all relations in table_infos, we also
check the return value of try_table_open(), and these two checks have
the same comment. I think we need more comments on why these two
checks are required.

In which case is schemaid InvalidOid and do we not call
try_table_open() (i.e., nulls[2] is false)?

It might make more sense to get-and-check the schemaid of each
relation when adding the table information to table_infos.

---
Looking at the regression tests in the patch, it tests the ALL TABLES
publication cases and the concurrently-dropped table is handled when
we call check the return value of try_table_open() but not
get_rel_namespace(). I think the test case itself is fine but we can
do the same test without adding a new injection point. For instance,

backend-1: begin;
backend-2: begin; lock table t_dropme in access exclusive mode;
backend-1: select * from pg_publication_tables;
backend-2: drop table t_dropme; commit;
backend-1: get an error "could not open relation with XXX"

---
If we use tuplestore instead of SRF, can we simplify the code as we
would not need publication_tables_state and the above check? It would
be only for the master, though.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com