Re: Added schema level support for publication.

On Thu, Sep 16, 2021 at 8:59 AM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Tuesday, September 14, 2021 4:39 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > I have handled this in the patch attached.
>
> Thanks for updating the patch.
> Here are some comments.
>
> 1)
> +static void
> +AlterPublicationSchemas(AlterPublicationStmt *stmt, Relation rel,
> ...
> + /*
> + * If the table option was not specified remove the existing tables
> + * from the publication.
> + */
> + if (!tables)
> + {
> + rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
> + PublicationDropTables(pubform->oid, rels, false, true);
> + }
>
>
> It seems not natural to drop tables in AlterPublication*Schemas*,
> I think we'd better do it in AlterPublicationTables.

I felt keeping the current way keeps it better to avoid additional
checks. Thoughts?

> 2)
> static void
> AlterPublicationTables(AlterPublicationStmt *stmt, Relation rel,
> ...
> + /*
> + * If ALL TABLES IN SCHEMA option was not specified remove the
> + * existing schemas from the publication.
> + */
> + List *pubschemas = GetPublicationSchemas(pubid);
> + PublicationDropSchemas(pubform->oid, pubschemas, false);
>
> Same as 1), Is it better to modify the schema list in AlterPublicationSchemas ?

This is similar to above.

> 3)
> static void
> AlterPublicationTables(AlterPublicationStmt *stmt, Relation rel,
> ...
> /* check if the relation is member of the schema list specified */
> RelSchemaIsMemberOfSchemaList(rels, schemaidlist, false);
>
> IIRC, The check here is to check the specified tables and schemas in the
> command. Personally, this seems a common operation which can be placed in
> function AlterPublication(). If we move this check to AlterPublication() and if
> comment 1) and 2) makes sense to you, then we don't need the new function
> parameters in AlterPublicationTables() and AlterPublicationSchemas().

I felt we can keep the checks as is currently, else we will have to
extra checks outside and addition calls for conversion from oid to
Relation like:
if (stmt->options)
AlterPublicationOptions(pstate, stmt, rel, tup);
else
{
if (relations)
{
if (stmt->action != DEFELEM_DROP)
{
List *rels = OpenTableList(relations);

/* check if relation is member of the schema list specified */
RelSchemaIsMemberOfSchemaList(rels, schemaidlist, false);
CloseTableList(rels);
}

AlterPublicationTables(stmt, rel, tup, relations,
list_length(schemaidlist));
}
if (schemaidlist)
AlterPublicationSchemas(stmt, rel, tup, schemaidlist,
list_length(relations));
}

Thoughts?

Regards,
Vignesh