Re: Support EXCEPT for ALL SEQUENCES publications

On Wed, 17 Jun 2026 at 12:37, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> ////////////////////
> Some review comments for v10-00001.
> ////////////////////
>
> ======
> doc/src/sgml/logical-replication.sgml
>
> 1.
> <xref linkend="logical-replication-sequences"/>. When a publication is
> created with <literal>FOR ALL TABLES</literal>, a table or set of tables can
> be explicitly excluded from publication using the
> - <link linkend="sql-createpublication-params-for-except-table"><literal>EXCEPT</literal></link>
> + <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>
> + clause. Similarly, when a publication is created with
> + <literal>FOR ALL SEQUENCES</literal>, a sequence or set of sequences can be
> + explicitly excluded from publication using the
> + <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>
> clause.
> </para>
>
> Yes, it is valid English to say "excluded from publication" (meaning
> won't be published); however, in most other documentation about
> EXCEPT, we refer to the PUBLICATION *object*, so it is worded like
> "excluded from the publication".
>
> So I think it should be /from publication/from the publication/
>
> (fix in 2 places in this file)
>
> ======
> src/backend/catalog/pg_publication.c
>
> 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/
>
> Please also fix the 1st "mentioned" in that comment.
>
> ======
> src/backend/commands/publicationcmds.c
>
> 3.
> + /* Process EXCEPT sequence list */
> + if (stmt->for_all_sequences && exceptseqs != NIL)
> + {
> + List *rels = OpenRelationList(exceptseqs);
> +
> + PublicationAddRelations(puboid, rels, true, NULL, RELKIND_SEQUENCE);
> + CloseRelationList(rels);
> + }
>
> if (stmt->for_all_tables)
> {
> /* Process EXCEPT table list */
> - if (exceptrelations != NIL)
> + if (excepttbls != NIL)
> {
> List *rels;
>
> - rels = OpenTableList(exceptrelations);
> - PublicationAddTables(puboid, rels, true, NULL);
> - CloseTableList(rels);
> + rels = OpenRelationList(excepttbls);
> + PublicationAddRelations(puboid, rels, true, NULL, RELKIND_RELATION);
> + CloseRelationList(rels);
>
> I felt that the fragments for "Process EXCEPT sequence list" and
> "Process EXCEPT table list" should look exactly the same, but they are
> currently formatted slightly differently. Normally, you might not
> write the first `if` as nested, but here I think consistency is more
> important.
>
> SUGGESTION
> if (stmt->for_all_sequences)
> {
> /* Process EXCEPT sequence list */
> if (exceptseqs != NIL)
> {
> List *rels = OpenRelationList(exceptseqs);
>
> PublicationAddRelations(puboid, rels, true, NULL, RELKIND_SEQUENCE);
> CloseRelationList(rels);
> }
> }
>
> if (stmt->for_all_tables)
> {
> /* Process EXCEPT table list */
> if (excepttbls != NIL)
> {
> List *rels = OpenRelationList(excepttbls);
>
> PublicationAddRelations(puboid, rels, true, NULL, RELKIND_RELATION);
> CloseRelationList(rels);
> }
> ...
> }
>
> ======
> src/bin/pg_dump/pg_dump.c
>
> 4.
> + {
> + if (relkind == RELKIND_SEQUENCE)
> + simple_ptr_list_append(&pubinfo[i].except_sequences, tbinfo);
> + else
> + simple_ptr_list_append(&pubinfo[i].except_tables, tbinfo);
> + }
>
> This code is building the `except_sequences` list in a block guarded
> by >= 190000. In fact, EXCEPT SEQUENCES won't exist until PG20. I'm
> not sure if this code should have a 20000 check... It may be harmless
> as-is if this can never happen for PG19.
>
Added a TODO to use version check for PG20

> ======
> src/bin/psql/describe.c
>
> describeOneTableDetails:
>
> 5.
> printfPQExpBuffer(&buf, "/* %s */\n",
> _("Get publications containing this sequence"));
> - appendPQExpBuffer(&buf, "SELECT pubname FROM pg_catalog.pg_publication p"
> + appendPQExpBuffer(&buf, "SELECT p.pubname FROM pg_catalog.pg_publication p"
> "\nWHERE p.puballsequences"
> "\n AND pg_catalog.pg_relation_is_publishable('%s')"
> + "\n AND NOT EXISTS (\n"
> + " SELECT 1\n"
> + " FROM pg_catalog.pg_publication_rel pr\n"
> + " WHERE pr.prpubid = p.oid AND\n"
> + " pr.prrelid = '%s')"
> "\nORDER BY 1",
> - oid);
> + oid, oid);
>
> 5a.
> IIUC, the "NOT EXISTS" part is for skipping the excluded sequences.
> And, you don't say "AND pr.prexcept" because it is redundant since
> the only way for a SEQUENCE to be individually in pg_publication_rel
> is if it is an excluded sequence.
>
> Is my understanding correct? I think it is too subtle -- it might be
> better to say "AND pr.prexcept" even if it is not strictly needed.
>
Yes, your understanding is correct.
For ALL SEQUENCES publication, the pg_publication_rel will have the
entry only if the sequence is specified in the EXCEPT list.
I didn't add "AND pr.prexcept" because the corresponding code for ALL
TABLES publications also does not use "AND pr.prexcept".
```
SELECT pubname\n"
" , NULL\n"
" , NULL\n"
"FROM pg_catalog.pg_publication p\n"
"WHERE p.puballtables AND
pg_catalog.pg_relation_is_publishable('%s')\n"
" AND NOT EXISTS (\n"
" SELECT 1\n"
" FROM
pg_catalog.pg_publication_rel pr\n"
" WHERE pr.prpubid = p.oid AND\n"
" (pr.prrelid = '%s' OR
pr.prrelid = pg_catalog.pg_partition_root('%s')))\n"
"ORDER BY 1;",
```
I think it will be consistent with EXCEPT (TABLE .. ) case. So, I
think we should not add "AND pr.prexcept".

> ~
>
> 5b.
> Anyway, this code should not com into play for PG19, because excluded
> sequences will be a PG20 feature... so I think that whole "AND NOT
> EXISTS" part needs another check.
>
Separated and added a TODO to change the version.

> ~~~
>
> 6.
> + /* Print publications where the sequence is in the EXCEPT clause */
> + if (pset.sversion >= 190000)
> + {
> + printfPQExpBuffer(&buf,
> + "SELECT p.pubname\n"
> + "FROM pg_catalog.pg_publication p\n"
> + "JOIN pg_catalog.pg_publication_rel pr ON p.oid = pr.prpubid\n"
> + "WHERE pr.prrelid = '%s' AND pr.prexcept\n"
> + "ORDER BY 1;", oid);
>
> This whole part will be a PG20 feature. I think you should add a
> "TODO" reminder comment here to flag/remind to modify this to check >=
> 200000 as soon as the PG19 version is bumped.
>
Added

> ~~~
>
> describePublications:
>
> 7.
> + if (puballsequences)
> + {
> + if (pset.sversion >= 190000)
> + {
> + /* Get sequences in the EXCEPT clause for this publication */
> + printfPQExpBuffer(&buf, "/* %s */\n",
> + _("Get sequences excluded by this publication"));
> + printfPQExpBuffer(&buf,
> + "SELECT n.nspname || '.' || c.relname\n"
> + "FROM pg_catalog.pg_class c\n"
> + " JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace\n"
> + " JOIN pg_catalog.pg_publication_rel pr ON c.oid = pr.prrelid\n"
> + "WHERE pr.prpubid = '%s' AND pr.prexcept AND c.relkind = 'S'\n"
> + "ORDER BY 1", pubid);
> + if (!addFooterToPublicationDesc(&buf, _("Except sequences:"),
> + true, &cont))
> + goto error_return;
> + }
> + }
>
> Similar to the previous review comment. EXCEPT SEQUENCES is a PG20
> feature, not PG19, so this should have a "TODO" comment reminder to
> change the version check to 200000 when the PG version is bumped.
>
Added a TODO
> ======
> src/test/subscription/t/037_except.pl
>
> 8.
> +# Subscribe to multiple publications with different EXCEPT sequence list
>
> /list/lists/
>
>
> ////////////////////
> Some review comments for patch v10-0002
> ////////////////////
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> Examples:
>
> 1.
> + Reset the publication to be <literal>ALL SEQUENCES</literal> with no
> + exclusions:
>
> Thanks for changing the above. "In passing", can you modify the "ALL
> TABLES" text to use the same wording? Otherwise, the ALL TABLES text
> will never be improved.

I have also addressed the remaining comments and attached the updated v11 patch.

Thanks,
Shlok Kyal