On Mon, Jun 12, 2017 at 10:21 PM, 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:
>>>> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>>>> It looks like relation_is_updatable() didn't get the message about
>>>>> partitioned tables. Thus, for example, information_schema.views and
>>>>> information_schema.columns report that simple views built on top of
>>>>> partitioned tables are non-updatable, which is wrong. Attached is a
>>>>> patch to fix this.
>>>
>>> Thanks for the patch, Dean.
>>>
>>>>> I think this kind of omission is an easy mistake to make when adding a
>>>>> new relkind, so it might be worth having more pairs of eyes looking
>>>>> out for more of the same. I did a quick scan of the rewriter code
>>>>> (prompted by the recent similar omission for RLS on partitioned
>>>>> tables) and I didn't find any more problems there, but I haven't
>>>>> looked elsewhere yet.
>>>
>>> As he mentioned in his reply, Ashutosh's proposal to abstract away the
>>> relkind checks is interesting in this regard.
>>>
>>> On 2017/06/12 17:29, Ashutosh Bapat wrote:
>>>> Changes look good to me. In order to avoid such instances in future, I
>>>> have proposed to bundle the conditions as macros in [1].
>>>
>>> It seems that Ashutosh forgot to include the link:
>>>
>>> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=AvCYh55Q@mail.gmail.com
>>
>> 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.
>
> 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 for bringing this list to a common place. I thought about rewording
the message, but it looks like a user would be interested in the list
of relkinds rather than their connecting property. Dean also seems to
be of the same opinion.
>
> In any case, unless another committer else wants it, and assuming no
> complaints with the concept, I'd be happy to take this.
>
Thanks. I will update the patches with the error message changes and
also add some more macros by first commitfest.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company