On Sat, Nov 13, 2021, at 12:00 AM, Bharath Rupireddy wrote: > On Sat, Nov 13, 2021 at 12:06 AM Euler Taveira <eu...@eulerto.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/
