Re: Support EXCEPT for TABLES IN SCHEMA publications

Hi Nisha.

Here are some review comments for patch v6-0002.

======
Commit message

1.
Extend the EXCEPT clause support to allow tables to be excluded when
adding a schema to a publication via ALTER PUBLICATION ... ADD:

~

/ADD:/ADD./

======
doc/src/sgml/ref/alter_publication.sgml

Synopsis.

2.
- TABLES IN SCHEMA { <replaceable
class="parameter">schema_name</replaceable> | CURRENT_SCHEMA } [, ...
]
+ TABLES IN SCHEMA { <replaceable
class="parameter">schema_name</replaceable> | CURRENT_SCHEMA } [
EXCEPT ( <replaceable
class="parameter">except_table_object</replaceable> [, ... ] ) ] [,
... ]

~

Probably needs to change to introduce the 'tables_in_schema' part,
same as in the CREATE PUBLICATION synopsis.

~~~

3.
+
+<phrase>and <replaceable
class="parameter">except_table_object</replaceable> is:</phrase>
+
+ [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]

Something is wrong. Now the synopsis has 'except_table_object' 2x.

~~~

4.
+ <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. <literal>EXCEPT</literal> is not supported with
+ <literal>DROP TABLES IN SCHEMA</literal>; instead, dropping a schema from
+ the publication automatically removes all of its associated
+ <literal>EXCEPT</literal> entries.
+ </para>

4a.
I didn't think you need to say it is a "schema-level" publication.
That much is obvious because it already says "TABLES IN SCHEMA".

~

4b.
Maybe do not say "is not supported", because IMO that implies it will
cause an error.

SUGGESTION (or something like this)
Using <literal>DROP TABLES IN SCHEMA</literal> on a publication will
automatically also remove any associated <literal>EXCEPT</literal>
entries.

~~~

EXCEPT

5.
+ <varlistentry>
+ <term><literal>EXCEPT ( <replaceable
class="parameter">except_table_object</replaceable> [, ... ]
)</literal></term>
+ <listitem>
+ <para>
+ Specifies tables to be excluded from a schema-level publication entry.
+ This clause may be used with <literal>ADD TABLES IN SCHEMA</literal>
+ and not with <literal>DROP TABLES IN SCHEMA</literal>. 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>
+ </listitem>
+ </varlistentry>
+

5a.
Oh! If this EXCEPT part was previously missing even for the "FOR ALL
TABLES", then IMO that is a separate bug that should be in another
thread and patched/fixed asap, then your patch should just make small
changes to to it.

~

5b.
I don't think you need to say "schema-level" here... Maybe reword like
"When used with ADD TABLES IN SCHEMA...". Anyway, all this wording
will need to change a bit after the aforementioned fix for "FOR ALL
TABLES EXCEPT" patched/pushed.

~~~

Examples

6.
+<programlisting>
+ALTER PUBLICATION sales_publication ADD TABLES IN SCHEMA sales EXCEPT
(TABLE sales.internal, sales.drafts);
+</programlisting>

It is OK left in the description, but IMO it is better if you don't
use the schema-qualified name in the actual code fragment.

======
src/backend/commands/publicationcmds.c

AlterPublicationSchemas:

7.
+ /*
+ * Increment the command counter so that is_schema_publication() in
+ * GetExcludedPublicationTables() can see the just-inserted schema
+ * rows when AlterPublicationExceptTables runs next.
+ */
+ CommandCounterIncrement();

Should this cut/paste common code be done afterwards just if
stmt->action == AP_AddObjects/SetObjects ?

~~~

AlterPublicationExceptTables:

8.
+ /*
+ * This function handles EXCEPT entries for schema-level publications
+ * only. For FOR ALL TABLES publications, EXCEPT entries are already
+ * processed by AlterPublicationTables().
+ */
+ if (schemaidlist == NIL && !is_schema_publication(pubid))
+ return;

Huh? It seems contrary to the function comment that was also talking
about "FOR ALL TABLES".

Should this function really be called something different like
'AlterPublicationSchemaExceptTables'?

~~~

9.
+ /*
+ * EXCEPT with SET is not supported: SET replaces the schema list but does
+ * not have a well-defined semantics for merging or replacing existing
+ * except entries. Users should DROP and re-ADD the schema with the
+ * desired EXCEPT list instead.
+ */
+ if (stmt->action == AP_SetObjects)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("EXCEPT clause is not supported with SET in ALTER PUBLICATION")));

9a.
Not sure about this comment saying "does not have a well-defined
semantics". Should you instead just have XXX comment to simply say
"Not yet implemented", because this is getting replaced later by your
patch 0003 I think.

SUGGESTION
XXX EXCEPT with SET is not currently implemented. Workaround: Users
should DROP and re-ADD the schema with the desired EXCEPT list.

~

9b.
The ereport should be temporarily (until patch 0003 is pushed) have
using an errhint to say the workaround.

~~~

10.
+ if (stmt->action == AP_AddObjects)
+ {
+ List *rels;
+ List *explicitrelids;
+ ListCell *lc;
+
+ rels = OpenTableList(exceptrelations);
+
+ explicitrelids = GetIncludedPublicationRelations(pubid,
+ PUBLICATION_PART_ROOT);
+
+ /*
+ * Validate that each excluded table is not also in the explicit table
+ * list (which would be contradictory).
+ */
+ foreach(lc, rels)

Can tidy this using a foreach_ptr look instead of 'lc'.

~~~

11.
+ if (list_member_oid(explicitrelids, relid))
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("table \"%s.%s\" cannot appear in both the table list and the
EXCEPT clause",
+ get_namespace_name(relns),
+ RelationGetRelationName(pri->relation)));

Make use of the new function to get fully qualified names and replace
\"%s.%s\" with \"%s\".

~~~

12.
+ /*
+ * For FOR ALL TABLES, EXCEPT entries are processed by
+ * AlterPublicationTables(), so merge them in. For TABLES IN SCHEMA,
+ * they are handled separately by AlterPublicationExceptTables().
+ */
+ if (stmt->for_all_tables)
+ relations = list_concat(relations, exceptrelations);
AlterPublicationTables(stmt, tup, relations, pstate->p_sourcetext,
schemaidlist != NIL);
AlterPublicationSchemas(stmt, tup, schemaidlist);
+ AlterPublicationExceptTables(stmt, tup, exceptrelations, schemaidlist);

Would it be simpler if AlterPublicationExceptTables was merged (or
delegated from) the AlterPublicationSchemas?

======
src/bin/pg_dump/t/002_pg_dump.pl

13.
+ 'CREATE PUBLICATION pub11' => {
+ create_order => 50,
+ create_sql =>
+ 'CREATE PUBLICATION pub11 FOR TABLES IN SCHEMA dump_test EXCEPT
(TABLE dump_test.test_table);',
+ regexp => qr/^
+ \QCREATE PUBLICATION pub11 WITH (publish = 'insert, update, delete,
truncate');\E
+ /xm,
+ like => { %full_runs, section_post_data => 1, },
+ },
+
+ 'ALTER PUBLICATION pub11 ADD TABLES IN SCHEMA dump_test EXCEPT
(dump_test.test_table)'
+ => {
+ regexp => qr/^
+ \QALTER PUBLICATION pub11 ADD TABLES IN SCHEMA dump_test EXCEPT
(TABLE ONLY test_table);\E
+ /xm,
+ like => { %full_runs, section_post_data => 1, },
+ },
+
+ 'CREATE PUBLICATION pub12' => {
+ create_order => 50,
+ create_sql =>
+ 'CREATE PUBLICATION pub12 FOR TABLES IN SCHEMA dump_test EXCEPT
(TABLE dump_test.test_table, dump_test.test_second_table);',
+ regexp => qr/^
+ \QCREATE PUBLICATION pub12 WITH (publish = 'insert, update, delete,
truncate');\E
+ /xm,
+ like => { %full_runs, section_post_data => 1, },
+ },
+
+ 'ALTER PUBLICATION pub12 ADD TABLES IN SCHEMA dump_test EXCEPT
(dump_test.test_table, dump_test.test_second_table)'
+ => {
+ regexp => qr/^
+ \QALTER PUBLICATION pub12 ADD TABLES IN SCHEMA dump_test EXCEPT
(TABLE ONLY test_table, TABLE ONLY test_second_table);\E
+ /xm,
+ like => { %full_runs, section_post_data => 1, },
+ },
+

Should not need to specify schema-qualified names in the CREATE
PUBLICATION or the ALTER PUBLICATION. I think a better test would have
one of each (e.g. don't qualify the 'dump_test.test_table') in any of
those SQL.

======
src/bin/psql/tab-complete.in.c

14.
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD", "TABLES",
"IN", "SCHEMA", MatchAny, "EXCEPT", "(", "TABLE", MatchAnyN) &&
ends_with(prev_wd, ','))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD", "TABLES",
"IN", "SCHEMA", MatchAny, "EXCEPT", "(", "TABLE", MatchAnyN) &&
!ends_with(prev_wd, ','))
+ COMPLETE_WITH(")");

I've not tested this, but the code looks the same. so I suspect this
suffers the same problem where it lists potentially all tables instead
of just the table of the current schema. Maybe this is an unavoidable
quirk...

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

15.
Should you add a some more test cases?

e.g. Pass with EXCEPT without the schema-qualified name.
e.g. Pass with multiple excepted tables.
e.g. Fail because non-existing table name.
e.g. Fail because table not in schema.
e.g. Fail syntax because missing keyword TABLE.
e.g. do a DROP TABLES IN SCHEMA to test that the except tables gove removed too

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