On Fri, Feb 27, 2026 at 12:10 PM vignesh C <[email protected]> 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) = @_;
~~~
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.
~~~
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."));
~~~
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/.
4b) Should we add examples for SET/DROP EXCEPT TABLE as well?
~~~
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?
~~~
--
Thanks,
Nisha
