On 12 June 2017 at 17:51, Joe Conway <mail@joeconway.com> wrote:
> On 06/12/2017 07:40 AM, Joe Conway wrote:
>> On 06/12/2017 01:49 AM, Amit Langote wrote:
>>> As he mentioned in his reply, Ashutosh's proposal to abstract away the
>>> relkind checks is interesting in this regard.
>>>
>> I have not looked at Ashutosh's patch yet, but it sounds like a good
>> idea to me.
>
> After looking I remain convinced - +1 in general.
>
Yes, I think this will probably help, but I worry that it will turn
into quite a large and invasive patch, and there are a number of
design choices to be made over the naming and precise set of macros.
Is this really PG10 material?
My initial thought, looking at the patch, is that it might be better
to have all the macros in one file to make them easier to maintain.
> One thing I would change -- in many cases there are ERROR messages
> enumerating the relkinds. E.g.:
> 8<-----------------
> if (!RELKIND_CAN_HAVE_COLUMN_COMMENT(relation->rd_rel->relkind))
> ereport(ERROR,
> (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> errmsg("\"%s\" is not a table, view, materialized view,
> composite type, or foreign table",
> 8<-----------------
>
> I think those messages should also be rewritten to make them less
> relkind specific and more clear, otherwise we still risk getting out of
> sync in the future. Maybe something like this:
> 8<-----------------
> "\"%s\" is not a kind of relation that can have column comments"
> 8<-----------------
> Thoughts?
>
-1. I think the existing error messages provide useful information
that you'd be removing. If you're worried about the error messages
getting out of sync, then wouldn't it be better to centralise them
along with the corresponding macros?
> In any case, unless another committer else wants it, and assuming no
> complaints with the concept, I'd be happy to take this.
>
OK, have at it.
Barring objections, I'll push my original patch and work up patches
for the other couple of issues I found.
Regards,
Dean