Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Tue, Sep 21, 2021 at 6:05 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Tue, Sep 21, 2021 at 4:12 PM vignesh C <vignesh21@gmail.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.

This is common code applicable for the following scenarios:
i) create publication pub1 for all tables in schema sch1, table sch1.test;
ii) create publication pub1 for table sch1.test, all tables in schema sch1;
iii) create publication pub1 for table sch1.test;
alter publication pub1 add all tables in schema sch1;

I have changed it to make it suitable for all the cases.

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 or
part of the specified schema list.",
get_namespace_name(relSchemaId)));

The v32 patch attached at [1] handles the above.
[1] - https://www.postgresql.org/message-id/CALDaNm1R-xbQvz4LU5OXu3KKwbWOz3uDcT_YjRU6V0R5FZDYDg%40mail.gmail.com

Regards,
Vignesh