Re: Added schema level support for publication.

On Mon, Oct 18, 2021 at 2:05 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Mon, Oct 18, 2021 at 3:14 PM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > On Saturday, October 16, 2021 1:57 PM houzj(dot)fnst(at)fujitsu(dot)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