Re: Review items for EXCEPT TABLE publication

> 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 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/