Re: Support EXCEPT for ALL SEQUENCES publications

On Fri, 12 Jun 2026 at 10:55, Peter Smith <smithpb2250(at)gmail(dot)com> 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