Re: Support EXCEPT for ALL SEQUENCES publications

On Tue, 2 Jun 2026 at 05:37, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shlok,
>
> Some review comments for patch v8-0002.
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> Examples.
>
> 1.
> Everywhere in these example descriptions where you say "EXCEPT" or
> "ALL SEQUENCES" etc, those should all be using SGML <literal> markup.
>
Fixed

> ~~~
>
> 2.
> +  <para>
> +   Replace the sequence list in the publication's EXCEPT clause:
> +<programlisting>
> +ALTER PUBLICATION mypublication SET ALL SEQUENCES EXCEPT (SEQUENCE seq1, seq2);
> +</programlisting></para>
> +
> +  <para>
> +   Reset the publication to be a ALL SEQUENCES publication with no excluded
> +   sequences:
> +<programlisting>
> +ALTER PUBLICATION mypublication SET ALL SEQUENCES;
> +</programlisting></para>
>
> 2a.
> Too wordy.
>
> /in the publication's EXCEPT clause/in the EXCEPT clause/
>
In the existing documentation we already have similar documentation:
  <para>
   Replace the table list in the publication's EXCEPT clause:
<programlisting>
ALTER PUBLICATION mypublication SET ALL TABLES EXCEPT (TABLE users,
departments);
</programlisting></para>

I think we should keep the wording consistent. Thoughts?

> ~
>
> 2b.
> The 1st example comment saying "Replace the..." doesn't really make
> sense because reading from top-to-bottom, this was not even publishing
> sequences.
>
> So, I think the "ALTER PUBLICATION mypublication SET ALL SEQUENCES
> EXCEPT (SEQUENCE seq1, seq2);" example should come *after* the "ALTER
> PUBLICATION mypublication SET ALL SEQUENCES;" example.
>
Fixed

> ~~~
>
> 3.
> +  <para>
> +   Replace the table and sequence list in the publication's EXCEPT clause:
> +<programlisting>
> +ALTER PUBLICATION mypublication SET ALL TABLES EXCEPT (TABLE users,
> departments), ALL SEQUENCES EXCEPT (SEQUENCE seq1, seq2);
>  </programlisting></para>
>
> Too wordy. Should be plural.
>
> /in the publication's EXCEPT clause:/in the EXCEPT clauses:/
>
I have kept the wording the same. Reason same as 2(a).
I have made it plural.

> ======
> src/backend/catalog/pg_publication.c
>
> GetIncludedPublicationRelations:
>
> On Wed, May 27, 2026 at 11:03 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Wed, 27 May 2026 at 09:08, Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > GetIncludedPublicationRelations:
> > >
> > > 4.
> > > /*
> > >  * Gets list of relation oids that are associated with a publication.
> > >  *
> > >  * This should only be used FOR TABLE publications, the FOR ALL TABLES/SEQUENCES
> > >  * should use GetAllPublicationRelations().
> > >  */
> > > List *
> > > GetIncludedPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt)
> > > {
> > > Assert(!GetPublication(pubid)->alltables);
> > >
> > > return get_publication_relations(pubid, pub_partopt, false, RELKIND_RELATION);
> > > }
> > >
> > > The comment does not match the code/assert. Shouldn't it also do
> > > assert !allsequences?
> > >
> > This function can be called for ALL SEQUENCES publication. For example
> > 'pg_get_publication_tables', 'InvalidatePubRelSyncCache', etc can call
> > this function for all sequence publications, but in this case the list
> > returned is empty. So, there is no overall impact.
> > So, we should not add an 'assert !allsequences'.
>
> Hmm. The reply seems contrary to the function comment that says "This
> should only be used FOR TABLE publications", so if not going to add an
> Assert then doesn't the function comment need fixing?
>
Yes, the function comment is not correct and a patch was already
proposed for the same in [1]. But it is a stale state there.
Since this patch is related to ALL SEQUENCES and EXCEPT, I think we
can update it here. I have updated the comment.

> ======
> src/backend/commands/publicationcmds.c
>
> get_delete_rels:
>
> 5.
>  /*
> - * Add or remove table to/from publication.
> + * Recreate list of tables/sequences to be dropped from the publication.
> + * To recreate the relation list for the publication, look for existing
> + * relations that do not need to be dropped.
> + *
> + * 'rels' contains the given list of relations, and 'oldrelids' contains
> + * the OIDs of existing relations in the publication identified by 'pubid'.
> + */
>
> Why say "recreate" everywhere? Can it just say "Returns the list of
> ...." instead of "Recreate ...".
>
> Also, I think this function comment needs to explain in more detail
> that this is called during ALTER SET. IIUC, the 'rels' is the reids
> you want to end up with in the publication; so those need to be
> compared with the 'oldrelids' to find which old ones are not wanted
> anymore. Those unwanted ones are what the function returns.
>
I have reworded the comment

> ~~~
>
> AlterPublicationRelations:
>
> 6.
>   /*
>   * Nothing to do if no objects, except in SET: for that it is quite
> - * possible that user has not specified any tables in which case we need
> - * to remove all the existing tables.
> + * possible that user has not specified any tables or sequences in which
> + * case we need to remove all the existing tables and sequences.
>   */
>
> Maybe this can be reworded more simply:
>
> SUGGESTION
> /*
> * Nothing to do if no objects were specified, unless this is a SET
> * command, which may need to remove all existing tables and sequences.
> */
Fixed

[1]:https://www.postgresql.org/message-id/CANhcyEUkV-T6cK142w9wfME9nobFHOvn1f4itJLMG-oR4QoPbQ%40mail.gmail.com

Thanks,
Shlok Kyal