Re: Fix race condition in pg_get_publication_tables with concurrent DROP TABLE

On Mon, Jun 15, 2026 at 4:21 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Tue, May 12, 2026 at 2:20 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > I reviewed the patch and here are some comments:
>
> Thank you for reviewing!
>
> > +/* 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.
>
> Done for pre-HEAD versions. For HEAD, I used the SRF approach.
>
> > ---
> > + /* 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.
>
> When get_rel_namespace() returns InvalidOid for the concurrently
> dropped tables, it costs additional syscache lookups plus index scans
> (cache miss). So, there's no harm from the correctness perspective.
>
> However, I would like to optimize this part of the code by 1/ gating
> it with InvalidOid checkup, 2/ moving the schemaid fetch closer to
> where it's being used i.e. inside if (!pub->alltables). I would like
> to do this only for HEAD, so, I attached it as a separate 0002 patch.
> For pre-HEAD versions, I removed this check and retained the
> try_table_open() fix.

I'm not sure skipping get_rel_namespace() contributes to a visible
performance gain. Rather I'm concerned that checking
SearchSysCacheExists2(PUBLICATIONNAMESPACEMAP) with
schemaid=InvalidOid could lead to unexpected results.

I'm concerned about the fact that the patch handles only the case
where the table doesn't have the column list. The tables in the result
not having the column list are guaranteed to be present since we do
the existence check for them but others are not. I guess it's reliable
if we could call try_table_open() for all tables in the table_infos,
and check its namespace using RelationGetNamespace(). I think it
doesn't lead to noticeable overheads in practice as we're already
calling table_open() for all tables in ALL TABLES publications or ALL
TABLE IN SCHEMA publications where users cannot specify the column
list. What do you think?

> > ---
> > 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.
>
> I implemented this idea for HEAD and it simplifies the code a bit.

Sorry, I should have mentioned that the changes to use tuplestore
instead of SRF should be PG20. I think that we should not such a
refactoring even PG19.

Regards,

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