Re: Support EXCEPT for ALL SEQUENCES publications

////////////////////
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.

======
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.

~

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.

~~~

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.

~~~

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.

======
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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia