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

On Sat, Nov 13, 2021 at 7:57 PM Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Sat, Nov 13, 2021 at 7:16 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> > 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.
>
> Yeah. The relid is available in Form_pg_class.
>
> Firstly, I didn't quite like the function
> errdetail_relkind_not_supported name to be too long here and adding to
> it the 2 or 3 parameters, in many places we are crossing 80 char
> limit. Above these, a function with one parameter is always better
> than function with 3 parameters.
>
> Having two functions isn't a big deal at all, I think we have many
> functions like that in the core (although I'm not gonna spend time
> finding all those functions, I'm sure there will be such functions).
>
> I would still go with with 2 functions:
>
> int errdetail_relkind_not_supported(Form_pg_class rd_rel);
> int errdetail_relkind_not_supported_v2(Oid relid, char relkind, char
> relpersistence);
>
> > 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.
>
> Since we are modifying it on the master, I think it is okay to have 2
> functions given the code simplification advantages we get with
> errdetail_relkind_not_supported(Form_pg_class rd_rel).
>
> I would even think further to rename "errdetail_relkind_not_supported"
> and have the following, because we don't have to be always descriptive
> in the function names. The errdetail would tell the function is going
> to give some error detail.
>
> int errdetail_relkind(Form_pg_class rd_rel);
> int errdetail_relkind_v2(Oid relid, char relkind, char relpersistence);
>
> or
>
> int errdetail_rel(Form_pg_class rd_rel);
> int errdetail_rel_v2(Oid relid, char relkind, char relpersistence);
>
> I prefer the above among the three function names.
>
> Thoughts?

PSA v11 patch with 2 APIs with much simpler parameters and small function
names:

int errdetail_rel(Form_pg_class rd_rel);
int errdetail_rel_v2(Oid relid, char relkind, char relpersistence);

Please review it.

Regards,
Bharath Rupireddy.