Re: Support EXCEPT for TABLES IN SCHEMA publications

Some review comments for v26*

======

//////////
Patch v26-0001
//////////

src/backend/commands/publicationcmds.c

ProcessSchemaExceptTables:

1.
+/*
+ * Also rejects a schema being mentioned more than once with an EXCEPT
+ * clause, even if the EXCEPT clauses are identical — much like
+ * OpenTableList() rejects "FOR TABLE t1(a), t1(a)" despite the column
+ * lists matching. A schema can still be mentioned multiple times, just
+ * not more than once with EXCEPT.
+ *
+ * Qualify unqualified EXCEPT table names with the given schema (rejecting
+ * any explicitly qualified with a different schema), and append them to
+ * *except_pubtables.
+ *
+ * schemaid: OID of the schema for this TABLES IN SCHEMA mention.
+ * except_tables: the EXCEPT list (or NIL) attached to this mention.
+ * pstate: parse state of the statement, used to report the error position of
+ * an offending EXCEPT entry.
+ * *schemas: accumulates all schema OIDs seen so far in this statement.
+ * *schemas_with_except: This is a subset of *schemas. Tracks which of
+ * the seen schemas of this statement had an EXCEPT clause.
+ * *except_pubtables: accumulates the (now schema-qualified) EXCEPT table
+ * entries across the whole statement.
+ */

Needs some adjustment. It doesn't make sense for the function comment
to start with "Also ..."

~~~

2.
+ * - CreatePublication() inserts the explicit tables and the EXCEPT tables in
+ * a single command, and rows inserted by the current command are not
+ * visible to later lookups within the same command.
+ *
+ * - In the partition case, the excluded root's row might not yet exist
+ * because the root may be added to the EXCEPT list later. Detecting such
+ * contradictions when adding the root to the EXCEPT clause would require
+ * traversing downward through the partition hierarchy, whereas the
+ * existing ancestor lookups only traverse upward from the relation being
+ * added.

2a.
/, and rows inserted/, but rows inserted/

~

2b.
/In the partition case/For partitions/

AFAICT this case is referring to something like: "FOR TABLE part,
TABLES IN SCHEMA EXCEPT (part_root)"

But, isn't that just a variation of the 1st case issue? e.g. where
table "part" is not yet visible for later lookup of "root", then you
wont be able to check integrity of the partition tree regardless of
the up/down traversal logic, so I wasn't sure why this 2nd case was
separately mentioned at all.

======

//////////
Patch v26-0004 (docs)
//////////

1.
+ <para>
+ Replace the schema list of <structname>sales_publication</structname> with
+ only schema <structname>sales</structname>, excluding only
+ <structname>sales.drafts</structname>. All other previously excluded tables
+ in schema <structname>sales</structname> are no longer excluded. Any schemas
+ previously in <structname>sales_publication</structname> are removed:
+<programlisting>
+ALTER PUBLICATION sales_publication SET TABLES IN SCHEMA sales EXCEPT
(TABLE drafts);
+</programlisting>
+ </para>

The last modification missed the point of my previous review comment.
My point was now that the first sentence is modified it should just
end with a colon (:), because and all the rest is redundant since IMO
it is just repeating the same.

e.g.

BEFORE
Replace the schema list of <structname>sales_publication</structname>
with only schema <structname>sales</structname>, excluding only
<structname>sales.drafts</structname>. All other previously excluded
tables in schema <structname>sales</structname> are no longer
excluded. Any schemas previously in
<structname>sales_publication</structname> are removed:

SUGGESTION
Replace the schema list of <structname>sales_publication</structname>
with only schema <structname>sales</structname>, excluding only
<structname>sales.drafts</structname>:

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