Re: Added schema level support for publication.

On Tue, Sep 21, 2021 at 9:03 AM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> On Fri, Sep 17, 2021 at 10:09 PM vignesh C <vignesh21(at)gmail(dot)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