On Sat, Nov 13, 2021 at 7:16 PM Euler Taveira <euler@eulerto.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?
Regards,
Bharath Rupireddy.