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: http://www.postgresql.org/mailpref/pgsql-hackers
