Re: BUG #19864: pg_get_publication_tables() gets cache lookup error after concurrent DROP PUBLICATION

On Thu, Nov 12, 2026 at 4:07 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Thu, Nov 12, 2026 at 3:42 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > On Thu, Nov 12, 2026 at 4:20 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Nov 12, 2026 at 9:30 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> > > >
> > > Thanks Ajin for the patch.
> > >
> > > I see one issue in the patch: currently, when a publication is dropped
> > > in between, we get incomplete output for the dropped pub. See this:
> > >
> > > With no drop-pub executed:
> > >
> > > postgres=# select * from pg_publication_tables;
> > > pubname | schemaname | tablename | attnames | rowfilter
> > > ---------+------------+-----------+----------+-----------
> > > pub | public | part1 | {a} |
> > > pub | public | tab1 | {i} |
> > > pub | public | tab2 | {i} |
> > > pub | public | tab3 | {i} |
> > > (4 rows)
> > >
> > >
> > > When pub is dropped after it has looped twice already, outcome:
> > >
> > > postgres=# select * from pg_publication_tables;
> > > pubname | schemaname | tablename | attnames | rowfilter
> > > ---------+------------+-----------+----------+-----------
> > > pub | public | part1 | {a} |
> > > pub | public | tab1 | {i} |
> > > (2 rows)
> > >
> > > I feel the second outcome is misleading and inappropriate. The better
> > > solution would be to skip the dropped pub completely or raise a clear
> > > error.
> > >
> > > There is more to it; the user may either run:
> > >
> > > select * from pg_publication_tables;
> > > Or
> > > select pg_get_publication_tables('pub2', 'pub');
> > >
> > > Ideally, in the first case (using `pg_publication_tables`), we should
> > > skip the dropped publication completely, while in the second case
> > > (using `pg_get_publication_tables('pub2', 'pub')`), an error seems
> > > more suitable since the user explicitly requested information about a
> > > specific publication.
> > >
> > > I am not sure about the implementation complexity in this case. If the
> > > implementation is tricky, consistent behaviour for both commands is
> > > fine, as this scenario seems rare. Let's see what others say about
> > > this.
> >
> > Hi Shveta,
> >
> > Thanks for the review. I agree that the partial output is misleading.
> >
> > Skipping the dropped publication completely in pg_publication_tables
> > turns out to be tricky. The view and an explicit call reach the same C
> > function with the same arguments (the view passes one publication name
> > per LATERAL call), so the function can’t tell them apart without
> > changing the view definition, which needs a catversion bump and can’t
> > be back-patched. Also, by the time the drop is noticed, some rows for
> > that publication may already have been returned, so skipping it
> > “completely” would mean rewriting the function completely.
>
> Okay, got your point.
>
> > Since this
> > bug exists in 16 through 18 and the scenario is rare, I’ve gone with
> > consistent behaviour for both commands, as you suggested.
>
> Okay.
>
> > The attached v2 raises a clear error when a publication is dropped
> > while its tables are being listed, both for the view and for explicit
> > calls:
> >
> > ERROR: publication “pub” was concurrently dropped
> >
> > That’s in line with what the view already does today when a
> > publication is dropped before its turn comes (ERROR: publication “pub”
> > does not exist). The only exception is the
> > pg_get_publication_tables(text[], oid) variant used by tablesync. It’s
> > called with pub_missing_ok = true and already ignores missing
> > publications on its first call, so it keeps skipping the row. The
> > isolation test covers both cases.
>
> Okay. I am fine with errors in both cases. I will review and verify in
> depth soon.
>

I verified the known cases, they work well. I will verify a bit more tomorrow.
A few code comments:

1)
I noticed that the same condition now produces 2 different msgs
depending on timing, which I initially thought was okay.

(Cases: If the publication is dropped before its name is resolved on
the first call, the result is publication "pub" does not exist. If
it's dropped later, while a row is being formed, the result is
publication "pub" was concurrently dropped).

But on rethinking, it will be better to make the messages consistent.
How about moving 'concurrently dropped' part to errdetail?

ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("publication \"%s\" does not exist", pubname),
errdetail("The publication was dropped concurrently.")));

2)
Also I feel, it will be better to separate the logic of finding
pub-name from core logic. Suggestion:

pub = get_publication_internal(table_info->pubid, true);
if (pub == NULL)
{
if (!pub_missing_ok)
ereport(ERROR,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("publication \"%s\" does not exist",
get_pubname_from_list(ptstate->pubs,
table_info->pubid)),
errdetail("The publication was dropped concurrently."));

table_close(rel, AccessShareLock);
continue;
}

static char *
get_pubname_from_list(List *pubs, Oid pubid)
{
foreach_ptr(Publication, p, pubs)
{
if (p->oid == pubid)
return p->name;
}

Assert(false);
return NULL;
}

thanks
Shveta