MIME-Version: 1.0
References: 
 <CAJpy0uB20MhJJEaPJdm31t4fykJ+fChA_76jU2P9HX5knbJvAA@mail.gmail.com>
 <CAD21AoCC8XuwfX62qKBSfHUAoww_XB3_84HjswgL9jxQy696yw@mail.gmail.com>
 <OS9PR01MB12149EA0C749BC29C7C949E32F544A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
 <CAD21AoBbZEshyaK0PeiF_J4_S75EfF=Gcs=C+X-osoVoUnawuQ@mail.gmail.com>
 <CAHut+PssG+sHeV+Xo0g=S7xBb9FgDPjHYDR4iSuOdYXDq-Psng@mail.gmail.com>
 <CAA4eK1LaSfAG7UAuy1xpnkWKM_YtrPuhbgAxYBFY3Sp_v_KqoQ@mail.gmail.com>
 <CAD21AoAb8E8krN63cY_U7RQs9v-zkqUZyKT_UVKDwKfExtvTBg@mail.gmail.com>
 <CAA4eK1K1GLR7DXSABayQE+pWM=v1ODD6haPYxuDhAYwJN5gjzg@mail.gmail.com>
 <CALDaNm2kvFahDDvdgCNo=Nv-COz_N5Xw8YmzQBN2bd3g=N81fQ@mail.gmail.com>
 <CAHut+PsCqTR_kQu5M1TqBjnE6KM5cO22aH8boHfpMa_gSJBmWg@mail.gmail.com>
 <CALDaNm2OOgmNOPpABUU+AXzHhfrLG9HMfSd3jfNe=t3dc-kp1Q@mail.gmail.com>
 <CAJpy0uCN4gfP7fSt__KdW5wYQ82650Z6L4YLnjRHZTQ1yir1mg@mail.gmail.com>
 <CALDaNm32+c6RTE5xR6sJ=MZGgwEtzjkxpov_Hu70MXfbvmN+=Q@mail.gmail.com>
 <CAHut+PtQbK9USLepyzArXFoNuLok1MsBu_Jg4UT=koZocombFw@mail.gmail.com>
 <CALDaNm1tKuU479T=winBqoMb3MzO3Mta2juk8W3t2R5ps0_zyg@mail.gmail.com>
 <CALDaNm3jpYs7ALcU6m5=Li=udidjZoW5dMpyCFs8QHGaf0S8+A@mail.gmail.com>
 <CAJpy0uCWS=ybBKG-kRAfdWEe1VBNj+VqpAUUoT8MPaNS7EggiA@mail.gmail.com>
 <CAA4eK1LMM-P4NatbkjG-96B7hHC7KYrJ8XTsCZQy0jLO9Qj4Bw@mail.gmail.com>
 <CAJpy0uAyf71QSYitBf4WbCYq22HDR6LPdxB12TpTgTRpczwphw@mail.gmail.com>
 <CAFiTN-s5PW121mBGKin20YEQpZkWefMehmP=v+0onzEaMQpwdw@mail.gmail.com>
 <CAA4eK1LBf5asit18HcqcFinOkdCjD6Lk2Eid9PDhtH6acwYb8w@mail.gmail.com>
 <CALDaNm3cdoT58E3QtYCwBbzyxYJjoS2k7Q0EgzR9ta6fyDGHSg@mail.gmail.com>
 <CAHut+PthJx_gZJNgF=mWSpkWjQJ58KyhrZ7D7CkX_TVq12wv7A@mail.gmail.com>
 <CANhcyEVLp5kbaVR4=nh1jR4YWqv7YpVx_SnYoshbnOrnY79_fg@mail.gmail.com>
 <CAA4eK1+4ZNF-MGheeTtYF9TdfNBnKKJ8DivWZsXBnuJVkqfa0g@mail.gmail.com>
 <CANhcyEUQvEK+HOH6Y8Fy30fNvC631ZopWKhwgskXjKnuXiGV5Q@mail.gmail.com>
In-Reply-To: 
 <CANhcyEUQvEK+HOH6Y8Fy30fNvC631ZopWKhwgskXjKnuXiGV5Q@mail.gmail.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Mon, 30 Mar 2026 13:31:33 +1100
Message-ID: 
 <CAHut+PuUnDZ4ki8nCK6SkHOn8iP6N1Vm24jzWtEqRG9a_GMxrw@mail.gmail.com>
Subject: Re: Skipping schema changes in publication
To: Shlok Kyal <shlok.kyal.oss@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, vignesh C <vignesh21@gmail.com>,
	Dilip Kumar <dilipbalaut@gmail.com>, shveta malik <shveta.malik@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
 Nisha Moond <nisha.moond412@gmail.com>,
	Ashutosh Sharma <ashu.coek88@gmail.com>,
 "David G. Johnston" <david.g.johnston@gmail.com>,
	"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, YeXiu <1518981153@qq.com>,
	Ian Lawrence Barwick <barwick@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@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%2BPuUnDZ4ki8nCK6SkHOn8iP6N1Vm24jzWtEqRG9a_GMxrw%40mail.gmail.com>
Precedence: bulk

Hi Shlok.

Here are some review comments for the patch v3-0001.

(not yet reviewed the test code)


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

synopsis:

1.
-    [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]
+    TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ]

This is still not correct because it is missing the ellipsis that is
needed within 'except_table_object'. e.g, the currently documented
synopsis would not permit FOR ALL TABLE EXCEPT (TABLE t1,t2,t3);

You might describe that using one of these ways:

i)
TABLE { [ ONLY ] table_name [ * ] } [, ...]

ii)
TABLE [ ONLY ] table_name [ * ] [, ...]

iii)
TABLE table [,...]
and table is:
[ ONLY ] table_name [ * ]

======
doc/src/sgml/ref/create_publication.sgml

synopsis:

2.
Same synopsis problem as described in the above review comment. It is
missing the ellipsis that is needed within 'except_table_object'.

~~~

EXCEPT:

3.
    <varlistentry id="sql-createpublication-params-for-except-table">
-    <term><literal>EXCEPT TABLE</literal></term>
+    <term><literal>EXCEPT</literal></term>
     <listitem>

The 'EXCEPT' clause is not really at the same level as all the other
ones, because it can only be applied to the FOR ALL TABLES. So, I felt
maybe 'EXCEPT' might make more sense as a sublist entry under the FOR
ALL TABLES clause.

Moving this would also eliminate some of the current repetition:
e.g. "Tables listed in EXCEPT clause are excluded from the publication."
e.g. "This clause specifies a list of tables to be excluded from the
publication."

======
src/backend/catalog/pg_publication.c

GetAllPublicationRelations:

4.
- * in EXCEPT TABLE clause.
+ * in EXCEPT clause.

/in EXCEPT clause/in the EXCEPT clause/

======
src/backend/parser/gram.y

5.
 static void preprocess_pubobj_list(List *pubobjspec_list,
     core_yyscan_t yyscanner);
+static void preprocess_except_pubobj_list(List *pubexceptobjspec_list,
+ core_yyscan_t yyscanner);

The new function and the parameter names do not match as they do in
the existing function. e.g. Should this new function instead be called
'preprocess_pubexceptobj_list'?

~~~

preprocess_except_pubobj_list:

6.
+ ListCell   *cell;
+ PublicationObjSpec *pubobj;

Why is this function referring to PublicationObjSpec instead of
PublicationExceptObjSpec. Is it correct?

~~~

7.
+ if (pubobj->pubobjtype == PUBLICATIONOBJ_CONTINUATION)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid publication object list"),
+ errdetail("TABLE must be specified before a standalone table."),
+ parser_errposition(pubobj->location));

If this was intended to mimic code from the existing function, then I
felt it should say "standalone table name." instead of "standalone
table.".

======
src/bin/pg_dump/pg_dump.c

8.
  if (++n_except == 1)
- appendPQExpBufferStr(query, " EXCEPT TABLE (");
+ appendPQExpBufferStr(query, " EXCEPT (TABLE ");
  else
- appendPQExpBufferStr(query, ", ");
+ appendPQExpBufferStr(query, ", TABLE ");

You don't need the TABLE keyword in 2 places like that.

SUGGESTION
if (++n_except == 1)
  appendPQExpBufferStr(query, " EXCEPT (");
else
  appendPQExpBufferStr(query, ", ");
appendPQExpBuffer(query, "TABLE ONLY %s", fmtQualifiedDumpable(tbinfo));

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

9.
  'CREATE PUBLICATION pub10' => {
  create_order => 92,
  create_sql =>
-   'CREATE PUBLICATION pub10 FOR ALL TABLES EXCEPT TABLE
(dump_test.test_inheritance_parent);',
+   'CREATE PUBLICATION pub10 FOR ALL TABLES EXCEPT (TABLE
dump_test.test_inheritance_parent);',
  regexp => qr/^
- \QCREATE PUBLICATION pub10 FOR ALL TABLES EXCEPT TABLE (ONLY
dump_test.test_inheritance_parent, ONLY
dump_test.test_inheritance_child) WITH (publish = 'insert, update,
delete, truncate');\E
+ \QCREATE PUBLICATION pub10 FOR ALL TABLES EXCEPT (TABLE ONLY
dump_test.test_inheritance_parent, TABLE ONLY
dump_test.test_inheritance_child) WITH (publish = 'insert, update,
delete, truncate');\E
  /xm,
  like => { %full_runs, section_post_data => 1, },

(Maybe this question is unrelated to the current patch.)

Why is the expected result explicitly including ONLY children like
that instead of saying "EXCEPT (TABLE
dump_test.test_inheritance_parent *)"?

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

10.
- else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
"TABLES", "EXCEPT", "TABLE", "(", MatchAnyN) && ends_with(prev_wd,
','))
+ else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
"TABLES", "EXCEPT", "(", "TABLE", MatchAnyN) && ends_with(prev_wd,
','))
  COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
- else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
"TABLES", "EXCEPT", "TABLE", "(", MatchAnyN) && !ends_with(prev_wd,
','))
+ else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
"TABLES", "EXCEPT", "(", "TABLE", MatchAnyN) && !ends_with(prev_wd,
','))
  COMPLETE_WITH(")");

The CREATE PUBLICATION seems to have tab-completion logic for the ','
separator and closing ')'. But I did not see any similar
tab-completion logic for the EXCEPT clause in ALTER PUBLICATION. Is
this maybe another issue independent of this patch?

======
src/test/regress/expected/publication.out
src/test/regress/sql/publication.sql
src/test/subscription/t/037_except.pl

(I will review these later)

======
GENERAL

11.
The file src/backend/commands/publicationcmds.c still refers to
"(EXCEPT TABLES)".

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