public inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Peter Smith <smithpb2250@gmail.com>
To: Nisha Moond <nisha.moond412@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Support EXCEPT for TABLES IN SCHEMA publications
Date: Fri, 22 May 2026 15:30:12 +1000
Message-ID: <CAHut+Ps3sghX4qv1jehqMgvZG7DmUoAFqgjmVc5xUy+v5kHN3w@mail.gmail.com> (raw)
In-Reply-To: <CABdArM79m7-CTf6KGGGU2QBydFtuonGgfxRSqk-vhwTsH8z1ow@mail.gmail.com>
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>
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.
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: smithpb2250@gmail.com, nisha.moond412@gmail.com, shveta.malik@gmail.com, amit.kapila16@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Support EXCEPT for TABLES IN SCHEMA publications
In-Reply-To: <CAHut+Ps3sghX4qv1jehqMgvZG7DmUoAFqgjmVc5xUy+v5kHN3w@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox