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

On Sat, Nov 13, 2021, at 12:00 AM, Bharath Rupireddy wrote:
> On Sat, Nov 13, 2021 at 12:06 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> > > Here's a rebased v8 patch. Please review it.
> >
> > This improves the user experience by increasing the granularity of error
> > reporting, and maps well with the precedent set in 81d5995b4. I'm marking this
> > Ready for Committer and will go ahead and apply this unless there are
> > objections.
> >
> > Shouldn't we modify errdetail_relkind_not_supported() to include relpersistence
> > as a 2nd parameter and move those messages to it? I experiment this idea with
> > the attached patch. The idea is to provide a unique function that reports
> > accurate detail messages.
>
> Thanks. It is a good idea to use errdetail_relkind_not_supported. I
> slightly modified the API to "int errdetail_relkind_not_supported(Oid
> relid, Form_pg_class rd_rel);" to simplify things and increase the
> usability of the function further. For instance, it can report the
> specific error for the catalog tables as well. And, also added "int
> errdetail_relkind_not_supported _v2(Oid relid, char relkind, char
> relpersistence);" so that the callers not having Form_pg_class (there
> are 3 callers exist) can pass the parameters directly.
Do we really need 2 functions? I don't think so. Besides that, relid is
redundant since this information is available in the Form_pg_class struct.

int errdetail_relkind_not_supported(Oid relid, Form_pg_class rd_rel);

My suggestion is to keep only the 3 parameter function:

int errdetail_relkind_not_supported(Oid relid, char relkind, char relpersistence);

Multiple functions that is just a wrapper for a central one is a good idea for
backward compatibility. That's not the case here.

--
Euler Taveira
EDB https://www.enterprisedb.com/