On Tue, Aug 4, 2026 at 2:11 PM Nisha Moond <[email protected]> wrote: > > On Fri, Jul 31, 2026 at 11:41 AM shveta malik <[email protected]> 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
