public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: Shlok Kyal <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Nisha Moond <[email protected]>
Cc: Ashutosh Sharma <[email protected]>
Cc: David G. Johnston <[email protected]>
Cc: Zhijie Hou (Fujitsu) <[email protected]>
Cc: YeXiu <[email protected]>
Cc: Ian Lawrence Barwick <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Skipping schema changes in publication
Date: Mon, 30 Mar 2026 13:31:33 +1100
Message-ID: <CAHut+PuUnDZ4ki8nCK6SkHOn8iP6N1Vm24jzWtEqRG9a_GMxrw@mail.gmail.com> (raw)
In-Reply-To: <CANhcyEUQvEK+HOH6Y8Fy30fNvC631ZopWKhwgskXjKnuXiGV5Q@mail.gmail.com>
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>
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
view thread (377+ messages) latest in thread
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Skipping schema changes in publication
In-Reply-To: <CAHut+PuUnDZ4ki8nCK6SkHOn8iP6N1Vm24jzWtEqRG9a_GMxrw@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