Re: Logical Replication - improve error message while adding tables to the publication in check_publication_add_relation

On Mon, Apr 5, 2021 at 7:19 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Mon, Apr 5, 2021 at 6:41 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> > Here's the v4 patch reabsed on the latest master, please review it further.
> >
> > /* UNLOGGED and TEMP relations cannot be part of publication. */
> > if (!RelationIsPermanent(targetrel))
> > - ereport(ERROR,
> > - (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > - errmsg("table \"%s\" cannot be replicated",
> > - RelationGetRelationName(targetrel)),
> > - errdetail("Temporary and unlogged relations cannot be replicated.")));
> > + {
> > + if (RelationUsesLocalBuffers(targetrel))
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("\"%s\" is a temporary table",
> > + RelationGetRelationName(targetrel)),
> > + errdetail("Temporary tables cannot be added to publications.")));
> > + else if (targetrel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("\"%s\" is an unlogged table",
> > + RelationGetRelationName(targetrel)),
> > + errdetail("Unlogged tables cannot be added to publications.")));
> > + }
> >
> > RelationIsPermanent(), RelationUsesLocalBuffers(), and
> > targetrel->rd_rel->relpersistence all refers to relpersistence. Hence, it is
> > not necessary to test !RelationIsPermanent().
>
> Done.
>
> > I would slightly rewrite the commit message to something like:
> >
> > Improve publication error messages
> >
> > Adding a foreign table into a publication prints an error saying "foo is not a
> > table". Although, a foreign table is not a regular table, this message could
> > possibly confuse users. Provide a suitable error message according to the
> > object class (table vs foreign table). While at it, separate unlogged/temp
> > table error message into 2 messages.
>
> Thanks for the better wording.
>
> Attaching v5 patch, please have a look.
>

We get the following error while adding an index:
create publication mypub for table idx_t1;
ERROR: "idx_t1" is an index

This error occurs internally from table_openrv function call, if we
could replace this with relation_openrv and then check the table kind,
we could throw a similar error message here too like the other changes
in the patch.

Regards,
Vignesh