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

On Tue, Oct 19, 2021 at 4:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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()?

Modified

> 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."

Modified

> 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.

I have removed these changes from this patch, I will post a patch for
this separately later.

> 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.

Modified

> 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"?

Modified

> 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?

Modified

> 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."?

Modified

> 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>?

Modified

> 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.

Modified

> 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.

Modified

Attached v43 patch has the fixes for the same.

Regards,
Vignesh