Re: Support EXCEPT for TABLES IN SCHEMA publications

Hi Nisha.

Some review comments for v14-0001.

======
src/test/regress/sql/publication.sql

1.
+-- ALTER TABLE ... SET SCHEMA on a table excluded by a schema publication:
+-- the schema-scoped exclusion is no longer meaningful once the table moves
+-- out of its schema, so the exclution is auto-dropped.

1a.
typo: /exclution/exclusion/

~

1b.
Exclusions cannot be "dropped" (at least, not by a user with the DROP
command). So even though this is correct for what is happening
internally, maybe from the user PoV it is better to say something like
"auto-removed" or just "removed".

(Same comment may also apply to the DEBUG logging when this happened).

~~~

2.
+-- Restore so the existing cleanup below still works
+ALTER TABLE public.testpub_tbl_s2 SET SCHEMA pub_test;
+DROP PUBLICATION testpub_schema_except_setsch;

Why not just fix the cleanup code? It's not "existing cleanup" anyway
-- it's part of this same patch.

////////////////////

Some review comments for v14-0004.

======

doc/src/sgml/ref/alter_publication.sgml:

1.
used with a publication defined with <literal>FOR TABLE</literal> or
<literal>FOR TABLES IN SCHEMA</literal>, replaces the list of tables/schemas
in the publication with the specified list; the existing tables or schemas
- that were present in the publication will be removed.
+ that were present in the publication will be removed. When
+ <literal>SET TABLES IN SCHEMA</literal> is used with an
+ <literal>EXCEPT</literal> clause, the excluded tables for each schema are
+ replaced with the specified list; if <literal>EXCEPT</literal> is omitted
+ for a schema, any existing exclusions for that schema are cleared.

"are cleared" or "are removed"... not sure what is better. Maybe
"cleared" is fine.

~~~

2.
+ <para>
+ The <literal>EXCEPT</literal> clause can be used with
+ <literal>ADD TABLES IN SCHEMA</literal> to exclude specific tables from a
+ schema-level publication.
+ </para>
+

This is talking about the 'ADD' variant. So really, it belongs as part
of that 2nd para: "The first two variants modify..." because that is
where ADD is discussed.

~~~

3.
+ <varlistentry>
+ <term><literal>EXCEPT</literal></term>
+ <listitem>
+ <para>
+ When used with <literal>ADD TABLES IN SCHEMA</literal>
+ or <literal>SET TABLES IN SCHEMA</literal>, specifies
+ tables to be excluded from the publication. Each named
+ table must belong to the schema specified in the same
+ <literal>TABLES IN SCHEMA</literal> clause. Table names may be
+ schema-qualified or unqualified; unqualified names are implicitly
+ qualified with the schema named in the same clause. See
+ <xref linkend="sql-createpublication"/> for further details on the
+ semantics of <literal>EXCEPT</literal>.
+ </para>
+ <para>
+ For <literal>FOR TABLES IN SCHEMA</literal> publications, the
+ <literal>EXCEPT</literal> clause is schema-scoped. If a table listed in
+ the <literal>EXCEPT</literal> clause is later moved to a different schema
+ using <command>ALTER TABLE ... SET SCHEMA</command>, the exclusion is
+ removed; the table will then be published if its new schema is part of a
+ publication. If the table is subsequently moved back to the original
+ schema, the exclusion is not restored, and must be re-established
+ explicitly using <command>ALTER PUBLICATION</command>. Dropping a table
+ always removes it from the <literal>EXCEPT</literal> clause,
regardless of
+ publication type.
+ </para>
+ </listitem>
+ </varlistentry>

3a.
TBH, I lost track of what the final decision was about keeping this EXCEPT part.
IIUC, you wanted to defer a decision until later, but I did not
understand the plan:
a) Should the EXCEPT part be present for now, but consider removing it later?
b) Should the EXCEPT part be absent for now, but consider adding it later?

I prefer what you have done, but either way, we need to ensure Shlok's
thread uses the same approach.

~

3b.
I think that last "Dropping a table..." sentence should be a separate
<para>, unrelated to renaming/moving tables. Also, you don't need to
say "regardless of publication type" -- that would be the assumption
unless you say otherwise.

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