Re: Support EXCEPT for ALL SEQUENCES publications

On Mon, 3 Aug 2026 at 13:15, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Some review comments for v23-0002
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> 1.
> + publication defined for <literal>ALL SEQUENCES</literal>. When
> + <literal>EXCEPT</literal> is specified with
> + <literal>SET ALL TABLES</literal> or
> + <literal>SET ALL SEQUENCES</literal>, it replaces the existing exclusion
> + list; if omitted, the existing exclusion list is cleared. See
> + <xref linkend="sql-alterpublication-params-except"/> for more details.
>
> The whole part "When ... for more details." maybe can be removed
> because now the EXCEPT parameter is saying this same information.
>
> ~~~
>
> 2.
> + <varlistentry id="sql-alterpublication-params-except">
> + <term><literal>EXCEPT</literal></term>
> + <listitem>
> + <para>
> + When used with <literal>SET ALL TABLES</literal> or
> + <literal>SET ALL SEQUENCES</literal>, specifies the tables or sequences
> + to be excluded from the publication. The specified tables or sequences
> + replaces any existing exclusions. If <literal>EXCEPT</literal>
> is omitted,
> + any existing exclusions are cleared.
> + </para>
>
> For the first sentence, is it better to use the same wording as on the
> CREATE PUBLICATION page?
>
> SUGGESTION
> Specifies the tables to be excluded from an ALL TABLES publication, or
> the sequences to be excluded from an ALL SEQUENCES publication.
>
> ======
> src/backend/commands/publicationcmds.c
>
> get_delete_rels:
>
> 1.
> + /*
> + * Validate the column list. If the column list changes, then
> + * the validation done here will be duplicated inside
> + * PublicationAddRelations(). The validation is cheap enough
> + * that that seems harmless.
> + */
> + Bitmapset *newcolumns = pub_collist_validate(newpubrel->relation,
> + newpubrel->columns);
>
> That "Validate the column list." comment seems slightly misleading
> because I don't think that is the real purpose here. IIUC, we don't
> really want to do validation; we're calling this function to get the
> Bitmapset, and the validation is just a side-effect.
>
> SUGGESTION
> Get the column list Bitmapset. Validation is a side effect and will be
> duplicated inside PublicationAddRelations() if the column list
> changed. That's cheap enough that it seems harmless.

I have addressed the above comments and attached the updated v24 patch.

Thanks,
Shlok Kyal