> 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 think it would be clearer and safer to keep the whole relation-validation
section, including the foreach, inside the post-lock !alltables branch.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
