On Thu, Sep 16, 2021 at 8:59 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Tuesday, September 14, 2021 4:39 PM vignesh C <vignesh21@gmail.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
