Re: Skipping schema changes in publication

Hi Shlok.

Some review comments for v36-0001.

======
1.
It seems that most of my v35 public/internal review comments from
around 8/JAN have not been addressed in v36.

======
doc/src/sgml/ref/create_publication.sgml

EXCEPT TABLE

2.
+ <para>
+ For partitioned tables, only the root partitioned table may be specified
+ in <literal>EXCEPT TABLE</literal>. Doing so excludes the root table and
+ all of its partitions from replication, regardless of the value of
+ <literal>publish_via_partition_root</literal>. The optional
+ <literal>*</literal> has no effect for partitioned tables.
+ </para>

2a.
AFAIK, the 'publish_via_partition_root' value has nothing to do with
EXCEPT(partition) in this patch, but why was it really necessary to
actually say that?

~

2b.
Should this be saying that "ONLY" also does not mean anything when a
partitioned table is specified?

======
src/backend/catalog/pg_publication.c

publication_add_relation:

3.
+ /*
+ * Handle the case where a partition is excluded by EXCEPT TABLE
+ */
+ if (pub->alltables && pri->except && targetrel->rd_rel->relispartition)
+ ereport(ERROR,
+ (errmsg("partition \"%s\" cannot be excluded using EXCEPT TABLE",
+ RelationGetRelationName(targetrel))));

Should we have a hintmsg here to ask/tell the user that they might
need to try EXCEPT the partitioned root instead?

======
src/backend/commands/tablecmds.c

ATExecAttachPartition:

4.
+ /* Check if the partiton is part of EXCEPT list of any publication */
+ GetRelationPublications(RelationGetRelid(attachrel), NULL, &except_pubids);
+ if (except_pubids != NIL)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot attach relation \"%s\" as partition because it is
part of EXCEPT list in publication",
+ RelationGetRelationName(attachrel))));

Maybe reword the message

SUGGESTION
... because it is named in a publication EXCEPT TABLE list

======
src/backend/utils/cache/relcache.c

RelationBuildPublicationDesc:

5.
+ /*
+ * Only the topmost ancestor of a partitioned table can be specified
+ * in EXCEPT TABLES clause of a FOR ALL TABLES publication. So fetch
+ * the publications excluding the topmost ancestor only.
+ */
+ GetRelationPublications(llast_oid(ancestors), NULL, &exceptpuboids);
+

I found that "So fetch..." sentence to be quite ambiguous.

SUGGESTION
... , so we only need to check the topmost ancestor.

======
.../t/037_rep_changes_except_table.pl

6.
+ ALTER TABLE sch1.t1 ATTACH PARTITION sch1.part2 FOR VALUES FROM
(101) TO (200);

Was there any reason to do this ALTER?
AFAIK, you could've said both PARTITION BY and PARTITION OF in the
original CREATE TABLE.

~~~

7.
+# Partititions cannot be excluded using EXCEPT TABLE

7a.
typo: /Partititions/Partitions/

~

7b.
TBH, I don't know if it was necessary to repeat these partition tests
all the time with/without publish_via_partition_root set, but if you
still think it is needed, then the comment should say that all
combinations are tested to demonstrate that the parameter has no
effect.

Meanwhile, the same goes for all the other tests too ... Given that
'publish_via_partition_root' has no impact on EXCEPT TABLE, then why
do we need so many combinations of tests to show that it has no
effect? IOW, many other parameters also have nothing to do with EXCEPT
TABLE, but we don't test those.

~~~

8.
+# Cannot attach partition that is part of EXCEPT list in publication

SUGGESTION
Cannot attach a partition that is named in the EXCEPT list of any publication

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