Re: Added schema level support for publication.

On Tue, Sep 21, 2021 at 9:03 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Fri, Sep 17, 2021 at 10:09 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Attached v29 patch has the fixes for the same.
> >
>
> Some minor comments on the v29-0002 patch:
>
> (1)
> In get_object_address_publication_schema(), the error message:
>
> + errmsg("publication tables of schema \"%s\" in publication \"%s\"
> does not exist",
>
> isn't grammatically correct. It should probably be:
>
> + errmsg("publication tables of schema \"%s\" in publication \"%s\" do
> not exist",

"does not exist" is used across the file. Should we keep it like that
to maintain consistency. Thoughts?

> (2)
> getPublicationSchemaInfo() function header.
> "string" should be "strings"
>
> BEFORE:
> + * nspname. Both pubname and nspname are palloc'd string which will be freed by
> AFTER:
> + * nspname. Both pubname and nspname are palloc'd strings which will
> be freed by

I will change it in the next version.

> (3)
> getPublicationSchemaInfo()
>
> In the case of "if (!(*nspname))", the following line should probably
> be added before returning false:
>
>    *pubname = NULL;

In case of failure we return false and don't access it. I felt we
could keep it as it is. Thoughts?

> (4)
> GetAllSchemasPublicationRelations() function header
>
> Shouldn't:
>
> + * Gets the list of all relations published by FOR ALL TABLES IN SCHEMA
> + * publication(s).
>
> actually say:
>
> + * Gets the list of all relations published by a FOR ALL TABLES IN SCHEMA
> + * publication.
>
> since it is for a specified publication?

I will change it in the next version.

> (5)
> I'm wondering, in CheckObjSchemaNotAlreadyInPublication(), instead of
> checking "checkobjtype" each loop iteration, wouldn't it be better to
> just use the same for-loop in each IF block?

I will be changing it to:
static void
CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
                    PublicationObjSpecType checkobjtype)
{
  ListCell   *lc;

  foreach(lc, rels)
  {
    Relation  rel = (Relation) lfirst(lc);
    Oid     relSchemaId = RelationGetNamespace(rel);

    if (list_member_oid(schemaidlist, relSchemaId))
    {
      if (checkobjtype == PUBLICATIONOBJ_REL_IN_SCHEMA)
        ereport(ERROR,
            errcode(ERRCODE_INVALID_PARAMETER_VALUE),
            errmsg("cannot add schema \"%s\" to publication",
                 get_namespace_name(relSchemaId)),
            errdetail("Table \"%s\" in schema \"%s\" is already part
of the publication, adding the same schema is not supported.",
                  RelationGetRelationName(rel),
                  get_namespace_name(relSchemaId)));
      else if (checkobjtype == PUBLICATIONOBJ_TABLE)
        ereport(ERROR,
            errcode(ERRCODE_INVALID_PARAMETER_VALUE),
            errmsg("cannot add relation \"%s.%s\" to publication",
                 get_namespace_name(relSchemaId),
                 RelationGetRelationName(rel)),
            errdetail("Table's schema \"%s\" is already part of the
publication.",
                  get_namespace_name(relSchemaId)));
    }
  }
}
After the change checkobjtype will be checked only once in case of error.

Regards,
Vignesh