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

On Thu, Nov 12, 2026 at 11:16 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
>
> 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

Thanks for the review and the testing, Shveta.
Both suggestions make sense, and the attached v3 does both. The error
now uses the same message as when the publication is already gone on
the first call, with the concurrent drop in the detail: ERROR:
publication “pub” does not exist DETAIL: The publication was dropped
concurrently. I also moved the name lookup into a separate helper,
get_pubname_from_list(), as you suggested. One small difference:
instead of Assert(false) followed by return NULL, the helper ends with
an elog(ERROR), so that a non-assert build never passes NULL to the %s
in the error message if that case were ever reached.

regards,
Ajin Cherian
Fujitsu Australia