Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Mon, Oct 18, 2021 at 5:53 PM vignesh C <vignesh21@gmail.com> wrote:
>

Few comments on latest set of patches:
===============================
1.
+/*
+ * Filter out the partitions whose parent tables was also specified in
+ * the publication.
+ */
+static List *
+filter_out_partitions(List *relids)

Can we name this function as filter_partitions()?

2.
+ /*
+ * If the publication publishes partition changes via their
+ * respective root partitioned tables, we must exclude partitions
+ * in favor of including the root partitioned tables. Otherwise,
+ * the function could return both the child and parent tables which
+ * could cause the data of child table double-published in
+ * subscriber side.
+ */

Let's slightly change the last part of the line in the above comment
as: "... which could cause data of the child table to be
double-published on the subscriber side."

3.
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
..
..
@@ -38,7 +40,6 @@
 #include "utils/builtins.h"
 #include "utils/catcache.h"
 #include "utils/fmgroids.h"
-#include "utils/inval.h"
 #include "utils/lsyscache.h"

Does this change belong to this patch? If not, maybe you can submit a
separate patch for this. A similar change is present in
publicationcmds.c as well, not sure if that is required as well.

4.
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
...
...
+#include "nodes/makefuncs.h"

Do we need to include this file? I am able to compile without
including this file.

v42-0003-Add-tests-for-the-schema-publication-feature-of-
5.
+-- pg_publication_tables
+SET client_min_messages = 'ERROR';
+CREATE SCHEMA sch1;
+CREATE SCHEMA sch2;
+CREATE TABLE sch1.tbl1 (a int) PARTITION BY RANGE(a);
+CREATE TABLE sch2.tbl1_part1 PARTITION OF sch1.tbl1 FOR VALUES FROM
(1) to (10);
+CREATE PUBLICATION pub FOR ALL TABLES IN SCHEMA sch2 WITH
(PUBLISH_VIA_PARTITION_ROOT=1);
+SELECT * FROM pg_publication_tables;
+
+DROP PUBLICATION pub;
+CREATE PUBLICATION pub FOR TABLE sch2.tbl1_part1 WITH
(PUBLISH_VIA_PARTITION_ROOT=1);
+SELECT * FROM pg_publication_tables;

Can we expand the above comment on the lines of: "Test the list of
partitions published"?

v42-0004-Add-documentation-for-the-schema-publication-fea
6.
+     <row>
+      <entry><link
linkend="catalog-pg-publication-namespace"><structname>pg_publication_namespace</structname></link></entry>
+      <entry>schema to publication mapping</entry>
+     </row>
+
      <row>
       <entry><link
linkend="catalog-pg-publication-rel"><structname>pg_publication_rel</structname></link></entry>
       <entry>relation to publication mapping</entry>
@@ -6238,6 +6243,67 @@ SCRAM-SHA-256$<replaceable><iteration
count></replaceable>:<replaceable>&l
   </table>
  </sect1>

+ <sect1 id="catalog-pg-publication-namespace">
+  <title><structname>pg_publication_namespace</structname></title>

At one place, the new catalog is placed after pg_publication_rel and
at another place, it is before it. Shouldn't it be before in both
places as we have a place as per naming order?

7.
The
+   <literal>ADD</literal> clause will add one or more tables/schemas to the
+   publication. The <literal>DROP</literal> clauses will remove one or more
+   tables/schemas from the publication.

Isn't it better to write the above as one line: "The
<literal>ADD</literal> and <literal>DROP</literal> clauses will add
and remove one or more tables/schemas from the publication."?

8.
+  <para>
+   Add some schemas to the publication:
+<programlisting>
+ALTER PUBLICATION sales_publication ADD ALL TABLES IN SCHEMA
marketing_june, sales_june;
+</programlisting>
+  </para>

Can we change schema names to just marketing and sales? Also, let's
change the description as:"Add schemas
<structname>marketing</structname> and <structname>sales</structname>
to the publication <structname>sales_publication</structname>?

9.
+    [ FOR ALL TABLES
+      | FOR <replaceable class="parameter">publication
object</replaceable> [, ... ] ]
     [ WITH ( <replaceable
class="parameter">publication_parameter</replaceable> [= <replaceable
class="parameter">value</replaceable>] [, ... ] ) ]
+
+<phrase>where <replaceable class="parameter">publication
object</replaceable> is one of:</phrase>

Similar to Alter Publication, here also we should use
publication_object instead of publication object.

10.
+  <para>
+   Create a publication that publishes all changes for tables "users" and
+   "departments" and that publishes all changes for all the tables present in
+   the schema "production":
+<programlisting>
+CREATE PUBLICATION production_publication FOR TABLE users,
departments, ALL TABLES IN SCHEMA production;
+</programlisting>
+  </para>
+
+  <para>
+   Create a publication that publishes all changes for all the tables
present in
+   the schemas "marketing" and "sales":

It is better to use <structname> before and </structname> after schema
names in above descriptions.

-- 
With Regards,
Amit Kapila.