Re: Added schema level support for publication.

On Fri, Oct 22, 2021 at 2:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Oct 21, 2021 at 6:47 PM vignesh C <vignesh21(at)gmail(dot)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/