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

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

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