On Wed, 12 Aug 2026 at 14:40, shveta malik <[email protected]> wrote: > > On Tue, Aug 11, 2026 at 2:28 PM vignesh C <[email protected]> wrote: > > > > > > Apart from these, the rest of the comments are also fixed in the v26 > > version attached. > > Thanks Vignesh. A few comments on v26-0001: > > 1) > > 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.
That is my understanding as well. I tested a few scenarios to determine whether there was an actual issue and found that the following cases were succeeding when they should have thrown an error: Scenario 1: CREATE PUBLICATION p FOR TABLES IN SCHEMA s1 EXCEPT (TABLE s1.t); ALTER PUBLICATION p ADD TABLE s1.t; Scenario 2: CREATE PUBLICATION p2 FOR TABLE s1.t; ALTER PUBLICATION p2 ADD TABLES IN SCHEMA s1 EXCEPT (TABLE s1.t); Both scenarios should have resulted in an error, but they currently succeed. I have addressed these cases as well. > 2) > ProcessSchemaExceptTables: > > + * Also rejects a schema being mentioned more than once with an EXCEPT > + * clause, even if the EXCEPT clauses are identical — much like > + * OpenTableList() rejects "FOR TABLE t1(a), t1(a)" despite the column > + * lists matching. A schema can still be mentioned multiple times, just > + * not more than once with EXCEPT. > + * > + * Qualify unqualified EXCEPT table names with the given schema (rejecting > + * any explicitly qualified with a different schema), and append them to > + * *except_pubtables. > > We changed the order of comments above, but missed to change these: > > 'Also rejects a schema ' -> Reject a schema > Qualify unqualified -> 'Also qualify unqualified EXCEPT table..' Modified > 3) > CheckExceptNotInTableList: > + * Both checks are performed here, on the lists supplied by the statement, > + * because at this point the catalog cannot answer either question. The > + * corresponding checks in publication_add_relation() and > + * check_publication_add_relation() rely on pg_publication_rel entries and > + * ancestor lookups, neither of which is sufficient in the cases below: > > Since we have changed comment prior to it, this too needs to be > changed as 'Both checks' does not make much sense. > > Suggestion: > Similar checks are present in publication_add_relation() and > check_publication_add_relation(), but they rely on pg_publication_rel > entries and ancestor lookups. Here, the checks are performed on the > object lists collected during the statement, as the catalog state is > not sufficient in the cases below: Modified The v27 version patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm0802S%3D557rKtYcV%2BoEbXFw23n%3DXtPHMCbKGSgOgOgk2A%40mail.gmail.com Regards, Vignesh
