Обсуждение: [HACKERS] PG10 Partitioned tables and relation_is_updatable()
Hi, 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. 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. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
On 06/11/2017 04:32 AM, Dean Rasheed 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. > 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. Yeah, I noticed the same while working on the RLS related patch. I did not see anything else in rewriteHandler.c but it is probably worth looking wider for other omissions. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 06/11/2017 08:55 AM, Joe Conway wrote: > On 06/11/2017 04:32 AM, Dean Rasheed 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. > >> 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. > > Yeah, I noticed the same while working on the RLS related patch. I did > not see anything else in rewriteHandler.c but it is probably worth > looking wider for other omissions. So in particular: src/include/utils/rel.h:318: 8<---------------------------- /** RelationIsUsedAsCatalogTable* Returns whether the relation should be treated as a catalog table* from the pov oflogical decoding. Note multiple eval of argument!*/ #define RelationIsUsedAsCatalogTable(relation) \ ((relation)->rd_options && \((relation)->rd_rel->relkind == RELKIND_RELATION || \ (relation)->rd_rel->relkind == RELKIND_MATVIEW)? \((StdRdOptions *) (relation)->rd_options)->user_catalog_table : false) 8<---------------------------- Does RELKIND_PARTITIONED_TABLE apply there? Will continue to poke around... Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 11 June 2017 at 16:59, Joe Conway <mail@joeconway.com> wrote: > On 06/11/2017 08:55 AM, Joe Conway wrote: >> Yeah, I noticed the same while working on the RLS related patch. I did >> not see anything else in rewriteHandler.c but it is probably worth >> looking wider for other omissions. > > So in particular: > > #define RelationIsUsedAsCatalogTable(relation) \ > ((relation)->rd_options && \ > ((relation)->rd_rel->relkind == RELKIND_RELATION || \ > (relation)->rd_rel->relkind == RELKIND_MATVIEW) ? \ > ((StdRdOptions *) (relation)->rd_options)->user_catalog_table : false) > 8<---------------------------- > > Does RELKIND_PARTITIONED_TABLE apply there? > Hmm, a quick experiment shows that it won't allow a partitioned table to be used as a user catalog table, although I'm not sure if that's intentional. If it is, it doesn't appear to be documented. I found another RLS-related omission -- RemoveRoleFromObjectPolicy() doesn't work for partitioned tables, so DROP OWNED BY <role> will fail if there are any partitioned tables with RLS. I also found another couple of omissions in PL/pgSQL -- plpgsql_parse_cwordtype() and build_row_from_class(), so for example %rowtype doesn't work for partitioned tables. That's it for my quick once-over the code-base, but I'm not confident that will have caught everything. Regards, Dean
On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > Hi, > > 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. > > 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. > Changes look good to me. In order to avoid such instances in future, I have proposed to bundle the conditions as macros in [1]. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >> Hi, >> >> 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 Thanks, Amit
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. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
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
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
On 06/12/2017 11:33 AM, Dean Rasheed wrote: > On 12 June 2017 at 17:51, Joe Conway <mail@joeconway.com> wrote: >> 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? I was wondering the same after responding. Possibly not. > 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. Yeah, that was my thought as well. >> 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? I guess that could work too. > Barring objections, I'll push my original patch and work up patches > for the other couple of issues I found. No objections here -- we definitely need to fix those. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Mon, Jun 12, 2017 at 2:19 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >>> Hi, >>> >>> 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 Sorry and thanks for providing the link. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
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
On Tue, Jun 13, 2017 at 12:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > 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? No this is not for PG10. > > 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. > Right now the macros are listed just below relkind enum in pg_class.h. Is that a good place or do you think, we should list them in a separate file? > > Barring objections, I'll push my original patch and work up patches > for the other couple of issues I found. No objections, the patch is good to go as is. Sorry for high-jacking this thread. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On 13 June 2017 at 05:50, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > On Tue, Jun 13, 2017 at 12:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >> 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. > > Right now the macros are listed just below relkind enum in pg_class.h. > Is that a good place or do you think, we should list them in a > separate file? > Yeah, I wondered about putting them in a separate file, but I think just below the relkind enum is probably the best place, so that people changing that enum immediately see the first set of related things to be updated. >> Barring objections, I'll push my original patch and work up patches >> for the other couple of issues I found. > > No objections, the patch is good to go as is. Sorry for high-jacking > this thread. > No worries. I missed that other thread initially, so it was useful to link the 2 threads together. Regards, Dean
On 13 June 2017 at 05:50, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > On Tue, Jun 13, 2017 at 12:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >> Barring objections, I'll push my original patch and work up patches >> for the other couple of issues I found. > > No objections, the patch is good to go as is. Sorry for high-jacking > this thread. > Thanks for the review. Patch pushed. Here are patches for the other 2 issues, with test cases showing how they currently fail on HEAD. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
Hi Dean, On 2017/06/14 2:29, Dean Rasheed wrote: > On 13 June 2017 at 05:50, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: >> On Tue, Jun 13, 2017 at 12:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >>> Barring objections, I'll push my original patch and work up patches >>> for the other couple of issues I found. >> >> No objections, the patch is good to go as is. Sorry for high-jacking >> this thread. >> > > Thanks for the review. Patch pushed. > > Here are patches for the other 2 issues, with test cases showing how > they currently fail on HEAD. Thanks again. Both patches look good to me. Regards, Amit