MIME-Version: 1.0
References: 
 <CABdArM5sw4Q1ZU8HGdo4BSc1A_+8xtUNq17j6wcir=yMUy19Cg@mail.gmail.com>
 <CAHut+PvnH8QHa035Skoh1e9jm_H08DO9fQ=F-NAMsEpYf0RZ2Q@mail.gmail.com>
 <CAJpy0uDu0LcNXcZCP0cR_LHqo+sau33KwPFHemmGVYf_JTxRBQ@mail.gmail.com>
 <CAA4eK1KbCWBmEXH-rhQjKgNwq=onZp8vRR-QkRhPpbKwL-kQdw@mail.gmail.com>
 <CAHut+Pvj4=GWoJEd4EBdp4pi6KxXQ46ioW=PV+=UktiXr2gCvg@mail.gmail.com>
 <CABdArM75F0A+DGP8AOt-_b_XREX40rvFid1jRjnr_+S5b51t8Q@mail.gmail.com>
 <CAJpy0uDTshb243L5yEYWB3uO-JrwSoRqQDNovh03K2GZuuR3Pg@mail.gmail.com>
 <CAJpy0uDy97ULmJUwPacAzc5u2seuPK6RXgCS1rnsW2MfR4eeSw@mail.gmail.com>
 <CABdArM6oXXXSAxxXFktTTfBf4kyxJCvdNtTbUZtSwJ=CepN+Xw@mail.gmail.com>
 <CAJpy0uBqM+fq7+g1ZRATuY16H10MFP9i25wfFCYCE5MGu+PE0Q@mail.gmail.com>
 <CABdArM4uKaS1coCQj6rAwMmHqU_cCJyEWNic-PFF1_ZjDDM82Q@mail.gmail.com>
 <CAHut+Pu5VNakf5JAhKM7T-P_q37eN1Qgv5nvZUe+8RAAT41y4g@mail.gmail.com>
 <CABdArM6WTm2gP4pcjVdHT1Nx6zdLKTq7nLPUkzZOhprc95a6Rw@mail.gmail.com>
 <CAHut+Ptthc1X-UA8-6zG-iFeCDuoNd+oJRBZ1eCnJ9RNOXjfBQ@mail.gmail.com>
 <CABdArM79m7-CTf6KGGGU2QBydFtuonGgfxRSqk-vhwTsH8z1ow@mail.gmail.com>
In-Reply-To: 
 <CABdArM79m7-CTf6KGGGU2QBydFtuonGgfxRSqk-vhwTsH8z1ow@mail.gmail.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Fri, 22 May 2026 15:30:12 +1000
Message-ID: 
 <CAHut+Ps3sghX4qv1jehqMgvZG7DmUoAFqgjmVc5xUy+v5kHN3w@mail.gmail.com>
Subject: Re: Support EXCEPT for TABLES IN SCHEMA publications
To: Nisha Moond <nisha.moond412@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHut%2BPs3sghX4qv1jehqMgvZG7DmUoAFqgjmVc5xUy%2Bv5kHN3w%40mail.gmail.com>
Precedence: bulk

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.