On Sat, Nov 13, 2021, at 12:00 AM, Bharath Rupireddy 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.