Re: Added schema level support for publication.

On Mon, Oct 18, 2021 at 2:05 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Hi,
>
> On Mon, Oct 18, 2021 at 3:14 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Saturday, October 16, 2021 1:57 PM houzj.fnst@fujitsu.com wrote:
> > > Based on the V40 patchset, attaching the Top-up patch which try to fix the
> > > partition issue in a cleaner way.
> >
> > Attach the new version patch set which merge the partition fix into it.
> > Besides, instead of introducing new function and parameter, just add the
> > partition filter in pg_get_publication_tables which makes the code cleaner.
> >
> > Only 0001 and 0003 was changed.
>
> I've reviewed 0001 and 0002 patch and here are comments:
>
> 0001 patch:
>
> +/*
> + * Get the list of publishable relation oids for a specified schema.
> + *
> + * Schema will be having both ordinary('r') relkind tables and partitioned('p')
> + * relkind tables, so two rounds of scan are required.
> + */
> +List *
> +GetSchemaPublicationRelations(Oid schemaid, PublicationPartOpt pub_partopt)
> +{
> +        Relation       classRel;
> +        ScanKeyData key[3];
> +        TableScanDesc scan;
>
> I think it's enough to have key[2], not key[3].

Modified

> BTW, this function does the table scan on pg_class twice in order to
> get OIDs of both normal tables and partitioned tables. But can't we do
> that by the single table scan? I think we can set a scan key for
> relnamespace, and check relkind inside a scan loop.

Modified

> ---
> +                ObjectsInPublicationToOids(stmt->pubobjects, pstate,
> &relations,
> +
> &schemaidlist);
> +
> +                if (list_length(relations) > 0)
> +                {
> +                        List      *rels;
> +
> +                        rels = OpenTableList(relations);
> +                        CheckObjSchemaNotAlreadyInPublication(rels,
> schemaidlist,
> +
> PUBLICATIONOBJ_TABLE);
> +                        PublicationAddTables(puboid, rels, true, NULL);
> +                        CloseTableList(rels);
> +                }
> +
> +                if (list_length(schemaidlist) > 0)
> +                {
> +                        /* FOR ALL TABLES IN SCHEMA requires superuser */
> +                        if (!superuser())
> +                                ereport(ERROR,
> +
> errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +                                                errmsg("must be
> superuser to create FOR ALL TABLES IN SCHEMA publication"));
> +
>
> Perhaps we can do a superuser check before handling "relations"? If
> the user doesn't have the permission, we don't need to do anything for
> relations.

Modified

> 0002 patch:
>
> postgres(1:13619)=# create publication pub for all TABLES in schema
> CURRENT_SCHEMA      pg_catalog          public              s2
> information_schema  pg_toast            s1
>
> Since pg_catalog and pg_toast cannot be added to the schema
> publication can we exclude them from the completion list?

Modified

Thanks for the comments, the attached v42 patch has the fixes for the same.

Regards,
Vignesh