Re: Support EXCEPT for TABLES IN SCHEMA publications

On Fri, Jul 31, 2026 at 11:41 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> Nisha,
> A few comments on v24-001:
>
> 1)
> + if (is_except)
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("cannot add table \"%s\" to publication \"%s\"",
> + RelationGetQualifiedRelationName(targetrel),
> + pub->name),
> + errdetail("The table is named in the publication's EXCEPT clause for
> schema \"%s\".",
> + get_namespace_name(RelationGetNamespace(targetrel))),
> + errhint("Change the EXCEPT clause using ALTER PUBLICATION ... SET
> TABLES IN SCHEMA ... EXCEPT.")));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("relation \"%s\" is already member of publication \"%s\"",
> + RelationGetRelationName(targetrel), pub->name)));
>
> Does ERRCODE_INVALID_PARAMETER_VALUE suits better in 'if-block' rather
> than ERRCODE_DUPLICATE_OBJECT?
> See similar error you added in CheckExceptNotInTableList():
>
> + if (exceptrelid == explicitrelid)
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("table \"%s\" cannot be both published and excluded",
> + RelationGetQualifiedRelationName(pri->relation)));
>

Changed to ERRCODE_INVALID_PARAMETER_VALUE in if-block.

> 2)
> +static void
> +ProcessSchemaExceptTables(Oid schemaid, List *except_tables,
>
> Since we have defined most of the arguments in comment section, we can
> add 'pstate' as well for the sake of completion.
>

Done.

> 3)
> Atop CheckExceptNotInTableList(), please add a comment indicating why
> this funciton is still needed when we already have similar checks in
> publication_add_relation and check_publication_add_relation.
>
> 4)
> I was debugging the flow to see why 'CheckExceptNotInTableList' is
> still needed when we have similar check in publication_add_relation().
> My analysis:
>
> publication_add_relation() accepts 'if_not_exists' i.e. add the new
> entry 'if already not present' else skip it (no error if it is a
> duplicate addition). Most flows pass it as 'true'. The current code
> skips raising error if if_not_exists=true and entry exists. It made
> sense earlier, but in our implementation, I feel it should still raise
> an error if entries are cross wired (i.e., if an exclusion is present
> and we are trying to add it as an inclusion, or vice versa). The
> 'if_not_exists' based 'skip logic' should only be exercised if the
> nature of existing entity is of same kind as user is trying to add.
> Let me know if you have different understanding.
>
> Based on this, I tried debugging:
>
> create publication pub1 for table s1.t1, tables in schema s1 except
> (table s1.t1);
>
> What changes I did:
> --I skipped CheckExceptNotInTableList() invoked from
> CreatePublication() so that flow directly hits the immediate next call
> PublicationAddTables for schmea's except entries.
> --In publication_add_relation(), I made this 'if_not_exists'
> correction (by simply making it false for debuggin purpose).
>
> The flow was as follows:
>
> --CREATE-SUb first added table s1.t1 with prexcept=false through
> PublicationAddTables.
>
> --It then added Schmea to pg_pub_namespace using PublicationAddSchemas
> and then invoked PublicationAddTables on except-table for the schema.
> This step (against my expectation) could not find the entry added in
> previous step (prexcept=false once) using CheckPublicationRelEntry()
> and thus could not hit the required error. Instead it hit more
> internal error:
> ERROR: duplicate key value violates unique constraint
> "pg_publication_rel_prrelid_prpubid_index"
>
> So it seems the catalog change from the first step was not visible in
> the second step. What am I missing? The rel-cache invalidation or
> something else?
>
> Nisha, can you debug and analyse in this direction? Expectation is to
> either get rid of CheckExceptNotInTableList() or to conclude that
> CheckExceptNotInTableList() is a reasonable addition to the code.
>

CheckExceptNotInTableList() was written to handle only CREATE
PUBLICATION ... EXCEPT.

IIUC you are not seeing the catalog change because of command-counter
MVCC, not relcache invalidation. CommandCounterIncrement() is what
makes the changes so far visible to later lookups —
CreatePublication() already does that once, for the new pg_publication
row.

So yes, we could add a CommandCounterIncrement() between the two
PublicationAddTables() calls, but I don't think it gets us to a better
place:
1. The error message would be misleading. The explicit tables are
added first, so it is the later EXCEPT insertion that finds the
existing row, and that row has prexcept = false — so we would report -
relation "%s" is already member of publication "%s" (with a hint
pointing at ALTER PUBLICATION ... SET) for a CREATE PUBLICATION
statement. This one is arguable, though — we could tweak the error
message accordingly.
2. It still doesn't solve the partition case. Visibility aside,
check_publication_add_relation() only looks upwards from the relation
being added ("is my root excluded?"). For
CREATE PUBLICATION p FOR TABLE part, TABLES IN SCHEMA s EXCEPT
(TABLE root);
the root's EXCEPT row does not exist yet when part is inserted, and
when root is inserted it isn't a partition, so no check runs at all.
Catching it there would need a new downward scan over every partition
of an excluded root, which is more work than comparing the lists we
already have in hand.

Since CreatePublication() has every table of the statement available
in memory, this separate function identifies the contradiction with no
catalog lookup at all. So IMO keeping CheckExceptNotInTableList() is
the better way; I have expanded the comment atop the function to
explain this. Let me know your thoughts.
~~~

Attached is the v25 patch set addressing all of the above, as well as
Shlok's and Peter's comments in [1] and [2].

[1] https://www.postgresql.org/message-id/CANhcyEXtX-tAMMMvsK%2Be1TUzUWT77%2BiwTbXPx-nM1Va_UjYTHg%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPumod7UW71WHO0GMA2qibG%2BTTsX7j5FJykAJgBwR73EdA%40mail.gmail.com

--
Thanks,
Nisha