On Mon, Jun 12, 2017 at 10:21 PM, Joe Conway <m...@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.rash...@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=avcyh...@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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to