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

On Fri, Oct 22, 2021 at 2:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 21, 2021 at 6:47 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> >
> > Thanks for the comments, the attached v45 patch has the fix for the same.
> >
>
> The first patch is mostly looking good to me apart from the below
> minor comments:

Let me share other minor comments on v45-0001 patch:

>
> 1.
> +  <para>
> +   The catalog <structname>pg_publication_namespace</structname> contains the
> +   mapping between schemas and publications in the database.  This is a
> +   many-to-many mapping.
>
> There are extra spaces after mapping at the end which are not required.

+   <literal>ADD</literal> and <literal>DROP</literal>  clauses will add and
+   remove one or more tables/schemas from the publication.  Note that adding
+   tables/schemas to a publication that is already subscribed to will require a

There is also an extra space after "adding".

-    [ FOR TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ] [, ...]
-      | FOR ALL TABLES ]
+    [ FOR ALL TABLES
+      | FOR <replaceable
class="parameter">publication_object</replaceable> [, ... ] ]

Similarly, after "TABLES".

+
+     <para>
+      Specifying a table that is part of a schema specified by
+      <literal>FOR ALL TABLES IN SCHEMA</literal> is not supported.
+     </para>

And, after "by".

---

+static void
+AlterPublicationSchemas(AlterPublicationStmt *stmt,
+                                                HeapTuple tup, List
*schemaidlist)
+{
(snip)
+                PublicationAddSchemas(pubform->oid, schemaidlist, true, stmt);
+        }
+
+        return;
+}

The "return" at the end of the function is not necessary.

---
+                        if (pubobj->name)
+                                pubobj->pubobjtype =
PUBLICATIONOBJ_REL_IN_SCHEMA;
+                        else if (!pubobj->name && !pubobj->pubtable)
+                                pubobj->pubobjtype = PUBLICATIONOBJ_CURRSCHEMA;
+                        else if (!pubobj->name)
+                                ereport(ERROR,
+                                                errcode(ERRCODE_SYNTAX_ERROR),
+                                                errmsg("invalid
schema name at or near"),
+
parser_errposition(pubobj->location));

I think it's better to change the last "else if" to just "else".

---
+
+                        if (schemarelids)
+                        {
+                                /*
+                                 * 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 data of the child table
+                                 * to be double-published on the
subscriber side.
+                                 *
+                                 * XXX As of now, we do this when a
publication has associated
+                                 * schema or for all tables publication. See
+                                 * GetAllTablesPublicationRelations().
+                                 */
+                                tables =
list_concat_unique_oid(relids, schemarelids);
+                                if (publication->pubviaroot)
+                                        tables =
filter_partitions(tables, schemarelids);
+                        }
+                        else
+                                tables = relids;
+
+                }

There is an extra newline after "table = relids;".

The rest looks good to me.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/