Re: Added schema level support for publication.

On Tue, Sep 21, 2021 at 4:12 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> > (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?
>

When it's singular, "does not exist" is correct.
I think currently only this case exists in the publication code.
e.g.
"publication \"%s\" does not exist"
"publication relation \"%s\" in publication \"%s\" does not exist"

But "publication tables" is plural, so it needs to say "do not exist"
rather than "does not exist".

> >
> > 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?
>

OK then, I might be being a bit pedantic.
(I was just thinking, strictly speaking, we probably shouldn't be
writing into the caller's pass-by-reference parameters in the case
false is returned)

> > (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.
>

OK.

One thing related to this code is the following:

i)
postgres=# create publication pub1 for all tables in schema sch1,
table sch1.test;
ERROR: cannot add relation "sch1.test" to publication
DETAIL: Table's schema "sch1" is already part of the publication.

ii)
postgres=# create publication pub1 for table sch1.test, all tables in
schema sch1;
ERROR: cannot add relation "sch1.test" to publication
DETAIL: Table's schema "sch1" is already part of the publication.

Notice that in case (ii), the same error message is used, but the
order of items to be "added" to the publication is the reverse of case
(i), and really implies the table "sch1.test" was added first, but
this is not reflected by the error message. So it seems slightly odd
to say the schema is already part of the publication, when the table
was actually listed first.
I'm wondering if this can be improved?

One idea I had was the following more generic type of message, but I'm
not 100% happy with the wording:

DETAIL: Schema "sch1" and one of its tables can't separately be
part of the publication.

Regards,
Greg Nancarrow
Fujitsu Australia