Re: Support EXCEPT for TABLES IN SCHEMA publications

On Tue, Aug 4, 2026 at 2:11 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> 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,

Okay, I see your point.

> which is more work than comparing the lists we
> already have in hand.

I agree. It is expensive as well.

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

I will reveiw the comment. But I agree with the overall approach.

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