Re: Added schema level support for publication.

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