On Fri, 12 Jun 2026 at 10:55, Peter Smith <[email protected]> wrote: > > Some review comments for v9-0001. > > ====== > src/backend/catalog/pg_publication.c > > GetExcludedPublicationTables: > > 1. > - Assert(GetPublication(pubid)->alltables); > + Assert(GetPublication(pubid)->alltables || > + GetPublication(pubid)->allsequences); > > Better to assign to a variable first, instead of calling GetPublication() 2x. > I agree. I've stored the result of GetPublication(pubid) in a local variable and declared it under USE_ASSERT_CHECKING, since it is only used by the assertion.
> ~~~ > > GetAllTablesPublications: > > 2. > - * For a FOR ALL TABLES publication, the returned list excludes > tables mentioned > - * in the EXCEPT clause. > + * For a FOR ALL TABLES publication, the returned list excludes tables > + * mentioned in the EXCEPT clause. For a FOR ALL SEQUENCES publication, > + * it excludes sequences mentioned in the EXCEPT clause. > > (2 times) > > /mentioned/specified/ or > /mentioned/named/ > > ~~~ > > 3. > + /* EXCEPT filtering applies to tables and sequences */ > + exceptlist = GetExcludedPublicationRelations(pubid, pubviaroot ? > + PUBLICATION_PART_ROOT : > + PUBLICATION_PART_LEAF); > > I don't think the comment is needed anymore. It was relevant before, > when there was a condition, but now there is no condition. > > ====== > src/backend/parser/gram.y > > preprocess_pubobj_list: > > 4. > - /* relation name or pubtable must be set for this type of object */ > - if (!pubobj->name && !pubobj->pubtable) > + /* relation name or pubrelation must be set for this type of object */ > + if (!pubobj->name && !pubobj->pubrelation) > ... > - /* convert it to PublicationTable */ > - PublicationTable *pubtable = makeNode(PublicationTable); > + /* convert it to PublicationRelation */ > + PublicationRelation *pubrelation = makeNode(PublicationRelation); > > Since you are changing these comments, you can uppercase them > in-passing for consistency. > /relation name/Relation name/ > /convert it/Convert it/ I have addressed the remaining comments in the v10 patches. Thanks, Shlok Kyal
v10-0002-Support-EXCEPT-for-ALL-SEQUENCES-in-ALTER-PUBLIC.patch
Description: Binary data
v10-0001-Support-EXCEPT-for-ALL-SEQUENCES-in-CREATE-PUBLI.patch
Description: Binary data
