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
Attachments:
teach-relation-is-updatable-about-partitioned-tables.patchapplication/octet-stream; name=teach-relation-is-updatable-about-partitioned-tables.patchDownload
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index 35ff8bb..331bc9f
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -2453,7 +2453,8 @@ relation_is_updatable(Oid reloid,
return 0;
/* If the relation is a table, it is always updatable */
- if (rel->rd_rel->relkind == RELKIND_RELATION)
+ if (rel->rd_rel->relkind == RELKIND_RELATION ||
+ rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
relation_close(rel, AccessShareLock);
return ALL_EVENTS;
@@ -2567,7 +2568,8 @@ relation_is_updatable(Oid reloid,
base_rte = rt_fetch(rtr->rtindex, viewquery->rtable);
Assert(base_rte->rtekind == RTE_RELATION);
- if (base_rte->relkind != RELKIND_RELATION)
+ if (base_rte->relkind != RELKIND_RELATION &&
+ base_rte->relkind != RELKIND_PARTITIONED_TABLE)
{
baseoid = base_rte->relid;
include_cols = adjust_view_column_set(updatable_cols,
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
new file mode 100644
index f6b51a5..971dddd
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2378,6 +2378,42 @@ alter table pt11 add a int not null;
alter table pt1 attach partition pt11 for values from (2) to (5);
alter table pt attach partition pt1 for values from (1, 2) to (1, 10);
create view ptv as select * from pt;
+select events & 4 != 0 AS upd,
+ events & 8 != 0 AS ins,
+ events & 16 != 0 AS del
+ from pg_catalog.pg_relation_is_updatable('pt'::regclass, false) t(events);
+ upd | ins | del
+-----+-----+-----
+ t | t | t
+(1 row)
+
+select pg_catalog.pg_column_is_updatable('pt'::regclass, 1::smallint, false);
+ pg_column_is_updatable
+------------------------
+ t
+(1 row)
+
+select pg_catalog.pg_column_is_updatable('pt'::regclass, 2::smallint, false);
+ pg_column_is_updatable
+------------------------
+ t
+(1 row)
+
+select table_name, is_updatable, is_insertable_into
+ from information_schema.views where table_name = 'ptv';
+ table_name | is_updatable | is_insertable_into
+------------+--------------+--------------------
+ ptv | YES | YES
+(1 row)
+
+select table_name, column_name, is_updatable
+ from information_schema.columns where table_name = 'ptv' order by column_name;
+ table_name | column_name | is_updatable
+------------+-------------+--------------
+ ptv | a | YES
+ ptv | b | YES
+(2 rows)
+
insert into ptv values (1, 2);
select tableoid::regclass, * from pt;
tableoid | a | b
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
new file mode 100644
index d89062c..6a9958d
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1125,6 +1125,16 @@ alter table pt1 attach partition pt11 fo
alter table pt attach partition pt1 for values from (1, 2) to (1, 10);
create view ptv as select * from pt;
+select events & 4 != 0 AS upd,
+ events & 8 != 0 AS ins,
+ events & 16 != 0 AS del
+ from pg_catalog.pg_relation_is_updatable('pt'::regclass, false) t(events);
+select pg_catalog.pg_column_is_updatable('pt'::regclass, 1::smallint, false);
+select pg_catalog.pg_column_is_updatable('pt'::regclass, 2::smallint, false);
+select table_name, is_updatable, is_insertable_into
+ from information_schema.views where table_name = 'ptv';
+select table_name, column_name, is_updatable
+ from information_schema.columns where table_name = 'ptv' order by column_name;
insert into ptv values (1, 2);
select tableoid::regclass, * from pt;
create view ptv_wco as select * from pt where a = 0 with check option;
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 of logical 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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:
/messages/by-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=AvCYh55Q@mail.gmail.com
Thanks,
Amit
--
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/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:
/messages/by-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:
/messages/by-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?
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
--
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/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:
/messages/by-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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:
/messages/by-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
teach-RemoveRoleFromObjectPolicy-about-partitioned-tables.patchtext/x-patch; charset=US-ASCII; name=teach-RemoveRoleFromObjectPolicy-about-partitioned-tables.patchDownload
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
new file mode 100644
index 4a75842..dad31df
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -474,7 +474,8 @@ RemoveRoleFromObjectPolicy(Oid roleid, O
rel = relation_open(relid, AccessExclusiveLock);
- if (rel->rd_rel->relkind != RELKIND_RELATION)
+ if (rel->rd_rel->relkind != RELKIND_RELATION &&
+ rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a table",
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
new file mode 100644
index e2ec961..26d28f2
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -3885,6 +3885,7 @@ RESET SESSION AUTHORIZATION;
CREATE ROLE regress_rls_dob_role1;
CREATE ROLE regress_rls_dob_role2;
CREATE TABLE dob_t1 (c1 int);
+CREATE TABLE dob_t2 (c1 int) PARTITION BY RANGE (c1);
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1 USING (true);
DROP OWNED BY regress_rls_dob_role1;
DROP POLICY p1 ON dob_t1; -- should fail, already gone
@@ -3892,6 +3893,9 @@ ERROR: policy "p1" for table "dob_t1" d
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
DROP OWNED BY regress_rls_dob_role1;
DROP POLICY p1 ON dob_t1; -- should succeed
+CREATE POLICY p1 ON dob_t2 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
+DROP OWNED BY regress_rls_dob_role1;
+DROP POLICY p1 ON dob_t2; -- should succeed
DROP USER regress_rls_dob_role1;
DROP USER regress_rls_dob_role2;
--
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
new file mode 100644
index 3ce9293..ba8fed4
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1740,6 +1740,7 @@ CREATE ROLE regress_rls_dob_role1;
CREATE ROLE regress_rls_dob_role2;
CREATE TABLE dob_t1 (c1 int);
+CREATE TABLE dob_t2 (c1 int) PARTITION BY RANGE (c1);
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1 USING (true);
DROP OWNED BY regress_rls_dob_role1;
@@ -1749,6 +1750,10 @@ CREATE POLICY p1 ON dob_t1 TO regress_rl
DROP OWNED BY regress_rls_dob_role1;
DROP POLICY p1 ON dob_t1; -- should succeed
+CREATE POLICY p1 ON dob_t2 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
+DROP OWNED BY regress_rls_dob_role1;
+DROP POLICY p1 ON dob_t2; -- should succeed
+
DROP USER regress_rls_dob_role1;
DROP USER regress_rls_dob_role2;
teach-plpgsql-about-partitioned-tables.patchtext/x-patch; charset=US-ASCII; name=teach-plpgsql-about-partitioned-tables.patchDownload
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
new file mode 100644
index a637551..cc09355
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -1761,7 +1761,8 @@ plpgsql_parse_cwordtype(List *idents)
classStruct->relkind != RELKIND_VIEW &&
classStruct->relkind != RELKIND_MATVIEW &&
classStruct->relkind != RELKIND_COMPOSITE_TYPE &&
- classStruct->relkind != RELKIND_FOREIGN_TABLE)
+ classStruct->relkind != RELKIND_FOREIGN_TABLE &&
+ classStruct->relkind != RELKIND_PARTITIONED_TABLE)
goto done;
/*
@@ -1987,7 +1988,8 @@ build_row_from_class(Oid classOid)
classStruct->relkind != RELKIND_VIEW &&
classStruct->relkind != RELKIND_MATVIEW &&
classStruct->relkind != RELKIND_COMPOSITE_TYPE &&
- classStruct->relkind != RELKIND_FOREIGN_TABLE)
+ classStruct->relkind != RELKIND_FOREIGN_TABLE &&
+ classStruct->relkind != RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("relation \"%s\" is not a table", relname)));
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
new file mode 100644
index 7ebbde6..35b83f7
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -5979,3 +5979,48 @@ LINE 1: SELECT (SELECT string_agg(id ||
^
QUERY: SELECT (SELECT string_agg(id || '=' || name, ',') FROM d)
CONTEXT: PL/pgSQL function alter_table_under_transition_tables_upd_func() line 3 at RAISE
+--
+-- Check type parsing and record fetching from partitioned tables
+--
+CREATE TABLE partitioned_table (a int, b text) PARTITION BY LIST (a);
+CREATE TABLE pt_part1 PARTITION OF partitioned_table FOR VALUES IN (1);
+CREATE TABLE pt_part2 PARTITION OF partitioned_table FOR VALUES IN (2);
+INSERT INTO partitioned_table VALUES (1, 'Row 1');
+INSERT INTO partitioned_table VALUES (2, 'Row 2');
+CREATE OR REPLACE FUNCTION get_from_partitioned_table(partitioned_table.a%type)
+RETURNS partitioned_table AS $$
+DECLARE
+ a_val partitioned_table.a%TYPE;
+ result partitioned_table%ROWTYPE;
+BEGIN
+ a_val := $1;
+ SELECT * INTO result FROM partitioned_table WHERE a = a_val;
+ RETURN result;
+END; $$ LANGUAGE plpgsql;
+NOTICE: type reference partitioned_table.a%TYPE converted to integer
+SELECT * FROM get_from_partitioned_table(1) AS t;
+ a | b
+---+-------
+ 1 | Row 1
+(1 row)
+
+CREATE OR REPLACE FUNCTION list_partitioned_table()
+RETURNS SETOF partitioned_table.a%TYPE AS $$
+DECLARE
+ row partitioned_table%ROWTYPE;
+ a_val partitioned_table.a%TYPE;
+BEGIN
+ FOR row IN SELECT * FROM partitioned_table ORDER BY a LOOP
+ a_val := row.a;
+ RETURN NEXT a_val;
+ END LOOP;
+ RETURN;
+END; $$ LANGUAGE plpgsql;
+NOTICE: type reference partitioned_table.a%TYPE converted to integer
+SELECT * FROM list_partitioned_table() AS t;
+ t
+---
+ 1
+ 2
+(2 rows)
+
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
new file mode 100644
index 60d1d38..f957d70
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4766,3 +4766,42 @@ ALTER TABLE alter_table_under_transition
DROP column name;
UPDATE alter_table_under_transition_tables
SET id = id;
+
+--
+-- Check type parsing and record fetching from partitioned tables
+--
+
+CREATE TABLE partitioned_table (a int, b text) PARTITION BY LIST (a);
+CREATE TABLE pt_part1 PARTITION OF partitioned_table FOR VALUES IN (1);
+CREATE TABLE pt_part2 PARTITION OF partitioned_table FOR VALUES IN (2);
+
+INSERT INTO partitioned_table VALUES (1, 'Row 1');
+INSERT INTO partitioned_table VALUES (2, 'Row 2');
+
+CREATE OR REPLACE FUNCTION get_from_partitioned_table(partitioned_table.a%type)
+RETURNS partitioned_table AS $$
+DECLARE
+ a_val partitioned_table.a%TYPE;
+ result partitioned_table%ROWTYPE;
+BEGIN
+ a_val := $1;
+ SELECT * INTO result FROM partitioned_table WHERE a = a_val;
+ RETURN result;
+END; $$ LANGUAGE plpgsql;
+
+SELECT * FROM get_from_partitioned_table(1) AS t;
+
+CREATE OR REPLACE FUNCTION list_partitioned_table()
+RETURNS SETOF partitioned_table.a%TYPE AS $$
+DECLARE
+ row partitioned_table%ROWTYPE;
+ a_val partitioned_table.a%TYPE;
+BEGIN
+ FOR row IN SELECT * FROM partitioned_table ORDER BY a LOOP
+ a_val := row.a;
+ RETURN NEXT a_val;
+ END LOOP;
+ RETURN;
+END; $$ LANGUAGE plpgsql;
+
+SELECT * FROM list_partitioned_table() AS t;
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers