Horton

Re: Skipping schema changes in publication

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: Shlok Kyal <shlok.kyal.oss@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, Amit Kapila <amit.kapila16@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>
Date: 2025-11-20T01:55:16Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix miscellaneous issues in EXCEPT publication clause.

  2. Change syntax of EXCEPT TABLE clause in publication commands.

  3. Add support for EXCEPT TABLE in ALTER PUBLICATION.

  4. Allow table exclusions in publications via EXCEPT TABLE.

  5. Add wait_for_subscription_sync for TAP tests.

Hi Shlok,

Some review comments for patch v28-0001.

======
src/backend/commands/publicationcmds.c

AlterPublicationReset:
1.
+ values[Anum_pg_publication_puballtables - 1] =
BoolGetDatum(PUB_DEFAULT_ALL_TABLES);
+ replaces[Anum_pg_publication_puballtables - 1] = true;
+
+ values[Anum_pg_publication_puballsequences - 1] =
BoolGetDatum(PUB_DEFAULT_ALL_SEQUENCES);
+ replaces[Anum_pg_publication_puballsequences - 1] = true;

The PUB_DEFAULT_xxx made sense for all the publication parameters, but
these are just flags that say if the ALL TABLES or ALL SEQUENCES
clauses are present in the command, so I'm not sure that "default"
macros are appropriate here.

e.g. it seems better to reset those using hardwired booleans:

values[Anum_pg_publication_puballtables - 1] = BoolGetDatum(false);
values[Anum_pg_publication_puballsequences - 1] = BoolGetDatum(false);

(This would also be consistent with what the function comment is saying)

======
src/include/catalog/pg_publication.h

2.
+/* default values for flags and publication parameters */
+#define PUB_DEFAULT_ACTION_INSERT true
+#define PUB_DEFAULT_ACTION_UPDATE true
+#define PUB_DEFAULT_ACTION_DELETE true
+#define PUB_DEFAULT_ACTION_TRUNCATE true
+#define PUB_DEFAULT_VIA_ROOT false
+#define PUB_DEFAULT_ALL_TABLES false
+#define PUB_DEFAULT_ALL_SEQUENCES false
+#define PUB_DEFAULT_GENCOLS PUBLISH_GENCOLS_NONE

As in the previous comment #1, I don't think we should define
PUB_DEFAULT_ALL_TABLES and PUB_DEFAULT_ALL_SEQUENCES. Also, change the
comment to remove the words "flags and".

======
src/test/regress/sql/publication.sql

3.
I don't think the parameter names should be written in uppercase
because that's not how they normally appear in the docs and examples.

~~~

4.
+-- Verify that 'ALL TABLES', 'ALL SEQUENCES' flag is reset

typo: /flag is reset/flags are reset/

~~~

5.
+-- Verify that associated tables, schemas and the publication parameters
+-- 'PUBLISH', 'PUBLISH_VIA_PARTITION_ROOT', and 'PUBLISH_GENERATED_COLUMNS'
+-- are removed from the publication after RESET

The comment makes it sound like parameters are "removed". Maybe some
rewording is needed.

SUGGESTION
Verify that a publication RESET removes the associated tables and
schemas, and sets default values for publication parameters 'publish',
'publish_via_partition_root', and 'publish_generated_columns'.

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