On Wed, 18 Mar 2026 at 18:51, vignesh C <[email protected]> wrote: > > On Wed, 18 Mar 2026 at 12:11, Peter Smith <[email protected]> wrote: > > > > Hi Vignesh. > > > > Some review comments for patch v65-0001. > > > > ====== > > doc/src/sgml/ref/alter_publication.sgml > > > > 1. > > It seems that SET ALL TABLES is not supported if the publication > > already has FOR TABLE. > > > > e.g > > alter publication pub1 set all tables; > > ERROR: publication "pub1" does not support ALL TABLES operations > > DETAIL: This operation requires the publication to be defined as FOR > > ALL TABLES/SEQUENCES or to be empty. > > > > e.g. > > alter publication pub2 set table t1; > > ERROR: publication "pub2" is defined as FOR ALL TABLES > > DETAIL: Tables or sequences cannot be added to or dropped from FOR > > ALL TABLES publications. > > > > I am not going to debate what rules are right or wrong. (Some rules do > > seem a bit ad-hoc to me, but maybe they are just erring on the safety > > side). But my point is that the documentation does not seem to say > > anything much about what the rules are > > > > e.g. it says "Adding/Setting any schema when the publication also > > publishes a table with a column list, and vice versa is not > > supported.", but OTOH it says nothing about what is > > supported/unsupported for SET ALL TABLES. > > Amit has already replied to this at [1]. > > > ====== > > src/backend/catalog/pg_publication.c > > > > 2. > > +/* > > + * Returns true if the publication has explicitly included relation (i.e., > > + * not marked as EXCEPT). > > + */ > > +bool > > +is_table_publication(Oid pubid) > > > > To me, an "explicitly included relation" is like when you say "FOR > > TABLE t1", where the "t1" is explicitly named. > > > > So it's not very clear whether you consider "FOR ALL TABLES" or a > > "FOR TABLES IN SCHEMA" publication explicitly includes tables or not? > > > > The function comment needs to be clearer. > > Amit has already replied to this at [1]. > > > ~~~ > > > > publication_add_relation: > > > > 3. > > + /* > > + * Determine whether EXCEPT tables require explicit relcache invalidation. > > + * > > + * For CREATE PUBLICATION with EXCEPT tables, invalidation is not needed, > > + * since it is handled when marking the publication as ALL TABLES. > > + * > > + * For ALTER PUBLICATION, invalidation is needed only when adding an EXCEPT > > + * table to a publication already marked as ALL TABLES. For publications > > + * that were originally empty or defined as ALL SEQUENCES and are being > > + * converted to ALL TABLES, invalidation is skipped here, as it is handled > > + * when marking the publication as ALL TABLES. > > + */ > > + inval_except_table = (stmt != NULL) && > > + (stmt->for_all_tables == pub->alltables); > > > > 3a. > > Why is this code done at the top of the function? Can you move it to > > be adjacent to where it is getting used? > > Modified > > > ~ > > > > 3b. > > I think 'alter_stmt' or 'alter_pub_stmt' might be a more informative > > name here, instead of the generic 'stmt' > > Modified > > > ====== > > src/backend/commands/publicationcmds.c > > > > 4. > > - TransformPubWhereClauses(rels, queryString, pubform->pubviaroot); > > + if (isexcept) > > + oldrelids = GetExcludedPublicationTables(pubid, > > + PUBLICATION_PART_ROOT); > > + else > > + { > > + oldrelids = GetIncludedPublicationRelations(pubid, > > + PUBLICATION_PART_ROOT); > > > > I felt there were some subtle things that the logic is using here: > > > > e.g. > > It seems that because this function is called... > > And because it is SET .... > > And because it is SET ALL TABLES ... > > Then, the tables can only be those in the EXCEPT TABLE list > > > > Maybe more comments about 'isexcept', and maybe an Assert(oldrelids != > > NIL); could help here (??) > > Updated comments > > > ~~~ > > > > AlterPublicationAllFlags: > > > > 5. > > + bool nulls[Natts_pg_publication]; > > + bool replaces[Natts_pg_publication]; > > + Datum values[Natts_pg_publication]; > > + bool dirty = false; > > + > > + if (!stmt->for_all_tables && !stmt->for_all_sequences) > > + return; > > + > > + pubform = (Form_pg_publication) GETSTRUCT(tup); > > + > > + memset(values, 0, sizeof(values)); > > + memset(nulls, false, sizeof(nulls)); > > + memset(replaces, false, sizeof(replaces)); > > > > AFAIK, these memsets are not needed if you just say "= {0}" where > > those vars are declared. > > modified > > > AlterPublication: > > > > 6. > > + relations = list_concat(relations, exceptrelations); > > AlterPublicationTables(stmt, tup, relations, pstate->p_sourcetext, > > schemaidlist != NIL); > > > > I did not quite understand this list_concat. > > > > Is this somehow asserting that `relations` must be empty when there > > were `exceptrelations` and vice versa, because other combinations are > > not supported by ALTER -- e.g. is this just a trick to so you can pass > > the same parameter to AlterPublicationTables? This seems related to > > the 'isexecpt' AlterPublicationTables function, which was also quite > > subtle. > > > > Bottom line is, I'm unsure what the logic is here, but it appears > > overly tricky to me. Would more comments help? > > Amit has already replied to this at [1]. > > > ====== > > src/backend/parser/gram.y > > > > 7. > > + * ALL TABLES [ EXCEPT TABLE ( table_name [, ...] ) ] > > > > The CREATE/ALTER docs synopsis says "[ ONLY ] table_name [ * ]" which > > is different from this comment. So are ONLY and * handled properly or > > not? > > It is handled, Is there any issue with the code? It is documented that > way to keep it consistent with CreatePublicationStmt and ALTER > PUBLICATION ADD/DROP/SET pub_obj. > > > ====== > > src/include/nodes/parsenodes.h > > > > 8. > > + bool for_all_tables; /* True if SET ALL TABLES is specified */ > > + bool for_all_sequences; /* True if SET ALL SEQUENCES is specified */ > > > > Maybe these comments do not need to mention the keyword "SET". > > > > That way, in future if/when ADD/DROP get implemented, then this code > > won't need to churn again. > > Modified > > The attached v66 version patch has the changes for the same.
While reviewing the patch I noticed couple of small improvements that could be done: 1) changed "may be used " to "can be used" as that is the convention followed: In addition, + <literal>SET ALL TABLES</literal> may be used to update the + <literal>EXCEPT TABLE</literal> list of a <literal>FOR ALL TABLES</literal> + publication. 2) This condition was slightly difficult to understand while reading, simplified it: Changed: - if (!pri->except) + inval_except_table = (alter_stmt != NULL) && + (alter_stmt->for_all_tables == pub->alltables); to: - if (!pri->except) + inval_except_table = (alter_stmt != NULL) && pub->alltables && + (alter_stmt->for_all_tables && pri->except); The attached v67 patch has the changes for the same. Regards, Vignesh
v67-0001-Add-support-for-EXCEPT-TABLE-in-ALTER-PUBLICATIO.patch
Description: Binary data
