On Wed, 17 Jun 2026 at 12:37, Peter Smith <[email protected]> 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
v11-0001-Support-EXCEPT-for-ALL-SEQUENCES-in-CREATE-PUBLI.patch
Description: Binary data
v11-0002-Support-EXCEPT-for-ALL-SEQUENCES-in-ALTER-PUBLIC.patch
Description: Binary data
