Re: Added schema level support for publication.

On Fri, Oct 22, 2021 at 1:03 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> 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.

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Removed it

I have made this change in the v46 patch attached at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm3kBrMO5EyEgK_TyOrBuw%2BRvdJ2mJfpWb5e8FbuKg2cQw%40mail.gmail.com

Regards,
Vignesh