Re: Review items for EXCEPT TABLE publication

On Thu, Sep 10, 2026 at 12:41 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
> > On Sep 10, 2026, at 13:39, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > I ran claude to identify issues related to the EXCEPT TABLE
> > publication changes. After reviewing the findings, I found the
> > following issues that need to be fixed.
> > Finding #1: ALTER PUBLICATION race
> > AlterPublicationOptions() performs validation using the publication
> > tuple before acquiring the publication lock. A concurrent ALTER
> > PUBLICATION ... SET ALL TABLES can change puballtables while the
> > second command is waiting for the lock. Since the tuple is not
> > re-fetched after acquiring the lock, the validation can proceed based
> > on stale state while subsequent catalog lookups see the updated state.
> > In an assert-enabled build, this can trigger the assertion in
> > GetIncludedPublicationRelations() because of
> > "Assert(!GetPublication(pubid)->alltables)" and crash the backend.
> >
> > Test to reproduce:
> > -- session 1: -- session 2:
> > CREATE PUBLICATION p;
> > BEGIN;
> > ALTER PUBLICATION p
> > SET ALL TABLES;
> > ALTER PUBLICATION p
> > SET
> > (publish_via_partition_root = false);
> > -- reads puballtables=false,
> > -- enters the branch
> > -- blocks while acquiring
> > -- AccessShareLock
> > COMMIT; -- resumes and assertion fires
> >
> > The fix is to check the current publication state after acquiring the
> > publication lock.
> >
> > Regards,
> > Vignesh
> > <v1-0001-Fix-ALTER-PUBLICATION-race-with-concurrent-SET-AL.patch><v1-0005-Prevent-unlogged-tables-in-publication-EXCEPT-cla.patch><v1-0003-Fix-missing-check-in-test_except_root_partition.patch><v1-0002-Fix-ALTER-PUBLICATION-validation-race.patch><v1-0004-Fix-test-to-use-a-fresh-subscription.patch>
>
> Since each commit addresses one finding, I’ll review and reply to the commits one by one.
>
> For 0001, I have two comments:
>
> 1. I don’t think this fix completely resolves the race. With GetPublication(pubform->oid)->alltables moved after acquiring the lock, S2 has to wait for S1 to commit, so it can see the alltables change made by S1.
>
> However, tup was fetched before AlterPublicationOptions() was called. After S1 commits its update, a new tuple version has been created, so the tup held by S2 is stale. I think S2 should re-fetch the publication tuple after acquiring the lock, so that both the validation and the subsequent catalog update operate on the current tuple version.
>
> 2. From a code-structure perspective, before this patch the relation-validation block was entered only when the publication was not FOR ALL TABLES. With this patch, the outer if is entered regardless of puballtables, and when GetPublication(pubform->oid)->alltables is true, root_relids is simply left empty.
>
> This is not a problem today because the only code that follows is the foreach, which does nothing when root_relids is empty. However, if more code is added after the loop in the future, it may unintentionally run for an ALL TABLESpublication as well.

I agree here.

> I think it would be clearer and safer to keep the whole relation-validation section, including the foreach, inside the post-lock !alltables branch.

+1.

thanks
Shveta