Re: Skipping schema changes in publication

Hi Vignesh.

Some review comments for v38-0001.

======

1. General. Patch structure

This patch set structure seems muddled to me; I think patch 0001
requires proper handling for EXCEPT (partitions), otherwise, it cannot
be independently committed.

It's my understanding that the goal is to try approach #1, but if that
proves too difficult, then the fallback would be approach #3. Yet
AFAICT this patch 0001 is neither -- I have no idea anymore what patch
0001 does for partitions; IIUC it looks like just old partition logic
from a few versions back (???).

Personally, I felt it would be better to combine 0001 + 0002 (approach
#3), then 0002 would be a patch that *replaces* approach #3 logic with
approach #1 logic. That way, 0001 is self-contained, and 0002 is an
evolution of the feature.

OTOH if the plan is that 0001 and (one of the) 0002 *must* be merged
prior to commit, then I would have expected that the 0001 has no
EXCEPT(partition) logic and not tests at all -- just maybe a bunch of
placeholders that patch 0002 would flesh out.

Anyway, the following review comments are based on the patch set as it
currently stands.

~~~

2. Still unaddressed comments for 3 weeks?

It seems most (all?) of my review comments from 8/January remain
unaddressed in this 0001 patch. Multiple versions have come and gone,
but the following review comments are still pending, according to my
records:

8/1 #1 (code review)
8/1 #3 (code review)
8/1 #1 (internal review)
8/1 #2a (internal review)
8/1 #2b (internal review)
8/1 #C2 (cosmetic review)
8/1 #C3 (cosmetic review)
8/1 #C4 (cosmetic review)
etc..

======
Commit Message

3.
The message needs to explain what this patch is doing regarding
partition tables and partitions. Currently, I have no idea. It seems
just old logic that is neither approach #1 nor approach #3.

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

4.
+     <para>
+      For partitioned tables, when
<literal>publish_via_partition_root</literal>
+      is set to <literal>true</literal>, specifying a root partitioned table in
+      <literal>EXCEPT TABLE</literal> excludes it and all its partitions from
+      replication. Specifying a leaf partition has no effect, as its
changes are
+      still replicated via the root partitioned table. When
+      <literal>publish_via_partition_root</literal> is set to
+      <literal>false</literal>, specifying a root partitioned table has no
+      effect, as changes are replicated via the leaf partitions. Specifying a
+      leaf partition excludes only that partition from replication.
The optional
+      <literal>*</literal> has no meaning for partitioned tables.
+     </para>

4a.
Huh? This is not at all my understanding of how either approach #1 or
approach #3 would work.

~

4b.
I thought the patch set is currently arranged to handle partitions in
the alternate patch 0002s.

So, I expected this doc fragment to be more like:
<para>
 For partitioned tables, TBA.
</para>

Later, your subsequent patch 0002 should replace this "TBA" part
according to approach #1 or #3 implementation.

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

publication_add_relation:

5.
+ /*
+ * Handle the case where a partition is excluded by EXCEPT TABLE while
+ * publish_via_partition_root = true.
+ */
+ if (pub->alltables && pub->pubviaroot && pri->except &&
+ targetrel->rd_rel->relispartition)
+ ereport(WARNING,
+ (errmsg("partition \"%s\" might be replicated as
publish_via_partition_root is \"%s\"",
+ RelationGetRelationName(targetrel), "true")));
+
+ /*
+ * Handle the case where a partitioned table is excluded by EXCEPT TABLE
+ * while publish_via_partition_root = false.
+ */
+ if (pub->alltables && !pub->pubviaroot && pri->except &&
+ targetrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ereport(WARNING,
+ (errmsg("partitioned table \"%s\" might be replicated as
publish_via_partition_root is \"%s\"",
+ RelationGetRelationName(targetrel), "false")));
+

Huh? IIUC, this is neither approach #1 nor approach #3.

======
src/backend/replication/pgoutput/pgoutput.c

get_rel_sync_entry:

6.
+ *
+ * If this is a FOR ALL TABLES publication and it has an EXCEPT
+ * TABLE list:
+ *
+ * 1. If pubviaroot is set and the relation is a partition, check
+ * whether the partition root is included in the EXCEPT TABLE
+ * list. If so, do not publish the change.
+ *
+ * 2. If pubviaroot is not set, check whether the relation itself
+ * is included in the EXCEPT TABLE list. If so, do not publish the
+ * change.
+ *
+ * This is achieved by keeping the variable "publish" set to
+ * false. And eventually, entry->pubactions will remain all false
+ * for this publication.

Again, this logic seems to describe neither approach #1 nor approach
#3. So, what is it (???)

======
src/test/subscription/t/037_rep_changes_except_table.pl

7.
IIUC, the patch set is arranged so that the 2 x patch 00002
implementations are for the alternate partition handling. So I was not
expecting patch 0001 to have any partition tests at all.

If 0001 is not implementing approach #1 or approach #3 logic, then
what are these tests testing?

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