Re: Skipping schema changes in publication

On Mon, 30 Mar 2026 at 08:02, Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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 [ * ]

Modified

> ======
> 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'.

The previous comment fixes this issue

> ~~~
>
> 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."

Amit has replied to this at [1].

> ======
> src/backend/catalog/pg_publication.c
>
> GetAllPublicationRelations:
>
> 4.
> - * in EXCEPT TABLE clause.
> + * in EXCEPT clause.
>
> /in EXCEPT clause/in the EXCEPT clause/

modified

> ======
> 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'?

The function has been removed now because of Hou-san's improvement
suggestion at [2].

> ~~~
>
> preprocess_except_pubobj_list:
>
> 6.
> + ListCell   *cell;
> + PublicationObjSpec *pubobj;
>
> Why is this function referring to PublicationObjSpec instead of
> PublicationExceptObjSpec. Is it correct?

The function has been removed now because of Hou-san's improvement
suggestion at [2].

> ~~~
>
> 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.".

The function has been removed now because of Hou-san's improvement
suggestion at [2].

> ======
> 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));

Modified

> ======
> 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 *)"?

This behavior is kept similar to how it is handled for table publication
ex:
CREATE TABLE test_inheritance_parent (col1 int NOT NULL, col2 int
CHECK (col2 >= 42));
CREATE TABLE test_inheritance_child (col1 int NOT NULL, CONSTRAINT
test_inheritance_child CHECK (col2 >= 142857) ) INHERITS
(test_inheritance_parent);
CREATE PUBLICATION pub10 FOR TABLE test_inheritance_parent;

Dump outputs the following:
--
-- Name: pub10 test_inheritance_child; Type: PUBLICATION TABLE;
Schema: public; Owner: vignesh
--
ALTER PUBLICATION pub10 ADD TABLE ONLY public.test_inheritance_child;

--
-- Name: pub10 test_inheritance_parent; Type: PUBLICATION TABLE;
Schema: public; Owner: vignesh
--
ALTER PUBLICATION pub10 ADD TABLE ONLY public.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?

Modified

> ======
> 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)".

Modified

The v4 version patch has the changes for the same.
The v4 version also addresses the comments provided by Peter at [3]
and Amit at [4].

[1] - https://www.postgresql.org/message-id/CAA4eK1Kb5Mg_MPDAZGb4PofRryXLjEFRd2k3aV%3DKCW-kmmkwvw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/TY4PR01MB169072D244423BED52C8D5B249452A%40TY4PR01MB16907.jpnprd01.prod.outlook.com
[3] - https://www.postgresql.org/message-id/CAHut%2BPvV_Yvm3VYWrbP%3DTL5_yvKUbQJbv%3DP3yt09odEV%2B1igYg%40mail.gmail.com
[4] - https://www.postgresql.org/message-id/CAA4eK1K0uHt-o%3DOAqh2qT7gDyef-BrHb%3DZ-94kCB8dH8r3SDeA%40mail.gmail.com

Regards,
Vignesh