Re: Skipping schema changes in publication

From: vignesh C <vignesh21(at)gmail(dot)com>
To: Nisha Moond <nisha(dot)moond412(at)gmail(dot)com>
Cc: shveta malik <shveta(dot)malik(at)gmail(dot)com>, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, YeXiu <1518981153(at)qq(dot)com>, Ian Lawrence Barwick <barwick(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Skipping schema changes in publication
Date: 2026-03-01 03:11:30
Message-ID: CALDaNm32g7c323M4mgZ5Wn8sbYp_=uQ6G_u0f9qfBCzuHP8jgQ@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, 27 Feb 2026 at 17:01, Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Fri, Feb 27, 2026 at 12:10 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Rest of the comments are addressed in the attached v51 version.
>
> Thanks for the patches, I am still testing the patches, but sharing a
> few initial review comments.
>
> 1) v51-0001: Bug in testcase - /t/037_rep_changes_except_table.pl
> The test_except_root_partition() is invoked twice with values true and
> false. However, from the logs it appears that both executions use
> publish_via_partition_root = 1.
> LOG output:
> 138 2026-02-27 12:26:51.167 IST client backend[72317]
> 037_rep_changes_except_table.pl LOG: statement: CREATE PUBLICATION
> tap_pub_part FOR ALL TABLES EXCEPT TABLE (sch1.t1) WITH
> (publish_via_partition_root = 1);
> ...
> ...
> 238 2026-02-27 12:26:51.600 IST client backend[72369]
> 037_rep_changes_except_table.pl LOG: statement: CREATE PUBLICATION
> tap_pub_part FOR ALL TABLES EXCEPT TABLE (sch1.t1) WITH
> (publish_via_partition_root = 1);
>
> It seems $pubviaroot is assigned using the argument count instead of
> the argument value, which causes both runs to behave the same.
> Fix is to replace the assignment as:
> 27 - my $pubviaroot = @_;
> 27 + my ($pubviaroot) = @_;

Fixed

> 2) v51-0001: File: src/backend/commands/tablecmds.c
>
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot attach table \"%s\" as partition because it is
> referenced in publication \"%s\" EXCEPT clause",
> + RelationGetRelationName(attachrel), pubnames.data),
> + errdetail("The publication EXCEPT clause cannot contain tables that
> are partitions.");
> + errhint("Remove the table from the publication EXCEPT clause before
> attaching it."));
> + }
>
> In the ereport() call, there appears to be a small typo. A comma
> should follow errdetail() instead of a semicolon.

Fixed

> 3) v51-0002: File: src/backend/commands/publicationcmds.c
> +
> + /* Check that user is allowed to manipulate the publication tables. */
> + if (excepttables && !pubform->puballtables)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("publication \"%s\" is defined as NON FOR ALL TABLES",
> + NameStr(pubform->pubname)),
> + errdetail("EXCEPT Tables cannot be added to or dropped from non FOR
> ALL TABLES publications."));
> }
>
> It seems the check is for both SET and DROP cases, but this patch is
> only for SET. Also, I find it a bit confusing, could be made clearer.
> Suggestion:
> - errmsg("publication \"%s\" is defined as NON FOR ALL TABLES",
> + errmsg("publication \"%s\" is not defined as FOR ALL TABLES",
> NameStr(pubform->pubname)),
> - errdetail("EXCEPT Tables cannot be added to or dropped from non FOR
> ALL TABLES publications."));
> + errdetail("EXCEPT TABLE cannot be used with publications that are
> not defined as FOR ALL TABLES."));

Modified with slight changes to keep it inline with other error messages there.

> 4) v51-0002 and v51-003:
> File: doc/src/sgml/ref/alter_publication.sgml
> ...
> - remove one or more tables/schemas from the publication. Note that adding
> - tables/schemas to a publication that is already subscribed to will
> require an
> + except tables/tables/schemas in the publication with the specified list; the
> + existing except tables/ tables/schemas that were present in the publication
> ...
> 4a) After adding “except tables” as a third option, the slash based
> formatting may be slightly confusing. It may be clearer to write as:
> ... the existing except tables, tables, or schemas that were present in ....
> ... The <literal>DROP</literal> clauses will remove one or more except
> tables, tables, or schemas from the publication....
>
> If the existing slash format is retained, there is a typo in "except
> tables/ tables/schemas", extra space after tables/.

Existing format was used, the typo issue was fixed.

> 4b) Should we add examples for SET/DROP EXCEPT TABLE as well?

Added them

> 5) Inconsistency in EXCEPT TABLE syntax
> In CREATE PUBLICATION, the syntax requires parentheses, for example
> EXCEPT TABLE (t1, t2), whereas in SET and DROP it is accepted only
> without parentheses, like - EXCEPT TABLE t1, t2;
>
> Should we keep the syntax consistent across all commands?

I’ll look into this separately and share my analysis soon.

The attached v53 version patch has the changes for the same.

Also the comments from [1] have been addressed.
[1] - https://www.postgresql.org/message-id/CAA4eK1L2LDps5AdtRYySQhmqSbKeUG2GinOKJCn7bvLEdKOoPg%40mail.gmail.com

Regards,
Vignesh

Attachment Content-Type Size
v53-0001-Skip-publishing-the-tables-specified-in-EXCEPT-T.patch application/octet-stream 77.0 KB
v53-0003-Support-DROP-EXCEPT-TABLE-in-ALTER-PUBLICATION.patch application/octet-stream 14.0 KB
v53-0002-Support-SET-EXCEPT-TABLE-in-ALTER-PUBLICATION.patch application/octet-stream 24.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tender Wang 2026-03-01 03:40:11 Re: [BUG?] estimate_hash_bucket_stats uses wrong ndistinct for avgfreq
Previous Message jian he 2026-03-01 01:53:04 Reject ADD CONSTRAINT NOT NULL if name mismatches existing domain not-null constraint