On Thu, Sep 10, 2026 at 12:41 PM Chao Li <[email protected]> wrote: > > > > > On Sep 10, 2026, at 13:39, vignesh C <[email protected]> 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
