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,
compositetype, 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?
In any case, unless another committer else wants it, and assuming no
complaints with the concept, I'd be happy to take this.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
