[HACKERS] map_partition_varattnos() and whole-row vars
Rajkumar Raghuwanshi reported [1] on the "UPDATE partition key" thread that whole-row vars don't play nicely with partitioned table operations. For example, when used to convert WITH CHECK OPTION constraint expressions and RETURNING target list expressions, it will error out if the expressions contained whole-row vars. That's a bug, because whole-row vars are legal in those expressions. I think the decision to output error upon encountering a whole-row in the input expression was based on the assumption that it will only ever be used to convert partition constraint expressions, which cannot contain those. So, we can relax that requirement so that its other users are not bitten by it. Attached fixes that. Adding this to the PG 10 open items list. Thanks, Amit [1] https://postgr.es/m/CAKcux6%3Dz38gH4K6YAFi%2BYvo5tHTwBL4tam4VM33CAPZ5dDMk1Q%40mail.gmail.com From b1954d7c2293bf9fc0d3ded30a742d7de0084082 Mon Sep 17 00:00:00 2001 From: amit Date: Wed, 26 Jul 2017 16:45:46 +0900 Subject: [PATCH] Fix map_partition_varattnos to sometimes accept wholerow vars It was thought that it would never encount wholerow vars in its input expressions (partition constraint expressions for example). But then it was used to convert expressions where wholerow vars are legal, such as, WCO constraint expressions and RETURNING target list members. So, add an argument to tell it whether or not to error on finding wholerows. Adds test in insert.sql and updatable_views.sql. Reported by: Rajkumar Raghuwanshi Report: https://postgr.es/m/CAKcux6%3Dz38gH4K6YAFi%2BYvo5tHTwBL4tam4VM33CAPZ5dDMk1Q%40mail.gmail.com --- src/backend/catalog/partition.c | 7 --- src/backend/commands/tablecmds.c | 2 +- src/backend/executor/nodeModifyTable.c| 16 src/include/catalog/partition.h | 3 ++- src/test/regress/expected/insert.out | 10 ++ src/test/regress/expected/updatable_views.out | 10 ++ src/test/regress/sql/insert.sql | 6 ++ src/test/regress/sql/updatable_views.sql | 9 + 8 files changed, 54 insertions(+), 9 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index e20ddce2db..b419466f2e 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -904,7 +904,8 @@ get_qual_from_partbound(Relation rel, Relation parent, */ List * map_partition_varattnos(List *expr, int target_varno, - Relation partrel, Relation parent) + Relation partrel, Relation parent, + bool wholerow_ok) { AttrNumber *part_attnos; boolfound_whole_row; @@ -921,7 +922,7 @@ map_partition_varattnos(List *expr, int target_varno, RelationGetDescr(parent)->natts, &found_whole_row); /* There can never be a whole-row reference here */ - if (found_whole_row) + if (found_whole_row && !wholerow_ok) elog(ERROR, "unexpected whole-row reference found in partition key"); return expr; @@ -1825,7 +1826,7 @@ generate_partition_qual(Relation rel) * in it to bear this relation's attnos. It's safe to assume varno = 1 * here. */ - result = map_partition_varattnos(result, 1, rel, parent); + result = map_partition_varattnos(result, 1, rel, parent, false); /* Save a copy in the relcache */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index bb00858ad1..e3eef88054 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -13738,7 +13738,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) constr = linitial(partConstraint); tab->partition_constraint = (Expr *) map_partition_varattnos((List *) constr, 1, - part_rel, rel); + part_rel, rel, false); /* keep our lock until commit */ if (part_rel != attachRel) heap_close(part_rel, NoLock); diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 77ba15dd90..cd4cf137e0 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1989,10 +1989,14 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) List *wcoExprs = NIL; ListCe
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On Wed, Jul 26, 2017 at 04:58:08PM +0900, Amit Langote wrote: > Rajkumar Raghuwanshi reported [1] on the "UPDATE partition key" thread > that whole-row vars don't play nicely with partitioned table operations. > > For example, when used to convert WITH CHECK OPTION constraint expressions > and RETURNING target list expressions, it will error out if the > expressions contained whole-row vars. That's a bug, because whole-row > vars are legal in those expressions. I think the decision to output error > upon encountering a whole-row in the input expression was based on the > assumption that it will only ever be used to convert partition constraint > expressions, which cannot contain those. So, we can relax that > requirement so that its other users are not bitten by it. > > Attached fixes that. > > Adding this to the PG 10 open items list. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Robert, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/07/26 16:58, Amit Langote wrote: > Rajkumar Raghuwanshi reported [1] on the "UPDATE partition key" thread > that whole-row vars don't play nicely with partitioned table operations. > > For example, when used to convert WITH CHECK OPTION constraint expressions > and RETURNING target list expressions, it will error out if the > expressions contained whole-row vars. That's a bug, because whole-row > vars are legal in those expressions. I think the decision to output error > upon encountering a whole-row in the input expression was based on the > assumption that it will only ever be used to convert partition constraint > expressions, which cannot contain those. So, we can relax that > requirement so that its other users are not bitten by it. > > Attached fixes that. Attached a revised version of the patch. Updated patch moves the if (found_whole_row) elog(ERROR) bit in map_partition_varattnos to the callers. Only the callers know if whole-row vars are not expected in the expression it's getting converted from map_partition_varattnos. Given the current message text (mentioning "partition key"), it didn't seem appropriate to have the elog inside this function, since it's callable from places where it wouldn't make sense. Thanks, Amit From 967bef28bb9ac25bb773934963f61c19c3ae7478 Mon Sep 17 00:00:00 2001 From: amit Date: Wed, 26 Jul 2017 16:45:46 +0900 Subject: [PATCH] Fix map_partition_varattnos to sometimes accept wholerow vars It was thought that it would never encount wholerow vars in its input expressions (partition constraint expressions for example). But then it was used to convert expressions where wholerow vars are legal, such as, WCO constraint expressions and RETURNING target list members. So, add an argument to tell it whether or not to error on finding wholerows. Adds test in insert.sql and updatable_views.sql. Reported by: Rajkumar Raghuwanshi Report: https://postgr.es/m/CAKcux6%3Dz38gH4K6YAFi%2BYvo5tHTwBL4tam4VM33CAPZ5dDMk1Q%40mail.gmail.com --- src/backend/catalog/partition.c | 17 ++--- src/backend/commands/tablecmds.c | 8 +++- src/backend/executor/nodeModifyTable.c| 18 ++ src/include/catalog/partition.h | 3 ++- src/test/regress/expected/insert.out | 10 ++ src/test/regress/expected/updatable_views.out | 10 ++ src/test/regress/sql/insert.sql | 6 ++ src/test/regress/sql/updatable_views.sql | 9 + 8 files changed, 68 insertions(+), 13 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index e20ddce2db..824898939e 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -904,10 +904,10 @@ get_qual_from_partbound(Relation rel, Relation parent, */ List * map_partition_varattnos(List *expr, int target_varno, - Relation partrel, Relation parent) + Relation partrel, Relation parent, + bool *found_whole_row) { AttrNumber *part_attnos; - boolfound_whole_row; if (expr == NIL) return NIL; @@ -915,14 +915,12 @@ map_partition_varattnos(List *expr, int target_varno, part_attnos = convert_tuples_by_name_map(RelationGetDescr(partrel), RelationGetDescr(parent), gettext_noop("could not convert row type")); + *found_whole_row = false; expr = (List *) map_variable_attnos((Node *) expr, target_varno, 0, part_attnos, RelationGetDescr(parent)->natts, - &found_whole_row); - /* There can never be a whole-row reference here */ - if (found_whole_row) - elog(ERROR, "unexpected whole-row reference found in partition key"); + found_whole_row); return expr; } @@ -1783,6 +1781,7 @@ generate_partition_qual(Relation rel) List *my_qual = NIL, *result = NIL; Relationparent; + boolfound_whole_row; /* Guard against stack overflow due to overly deep partition tree */ check_stack_depth(); @@ -1825,7 +1824,11 @@ generate_partition_qual(Relation rel) * in it to bear this relation's attnos. It's safe to assume varno = 1 * here. */ - result = map_partition_
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 28 July 2017 at 11:17, Amit Langote wrote: > On 2017/07/26 16:58, Amit Langote wrote: >> Rajkumar Raghuwanshi reported [1] on the "UPDATE partition key" thread >> that whole-row vars don't play nicely with partitioned table operations. >> >> For example, when used to convert WITH CHECK OPTION constraint expressions >> and RETURNING target list expressions, it will error out if the >> expressions contained whole-row vars. That's a bug, because whole-row >> vars are legal in those expressions. I think the decision to output error >> upon encountering a whole-row in the input expression was based on the >> assumption that it will only ever be used to convert partition constraint >> expressions, which cannot contain those. So, we can relax that >> requirement so that its other users are not bitten by it. >> >> Attached fixes that. > > Attached a revised version of the patch. > > Updated patch moves the if (found_whole_row) elog(ERROR) bit in > map_partition_varattnos to the callers. Only the callers know if > whole-row vars are not expected in the expression it's getting converted > from map_partition_varattnos. Given the current message text (mentioning > "partition key"), it didn't seem appropriate to have the elog inside this > function, since it's callable from places where it wouldn't make sense. create table foo (a int, b text) partition by list (a); create table foo1 partition of foo for values in (1); create table foo2(b text, a int) ; alter table foo attach partition foo2 for values in (2); postgres=# insert into foo values (1, 'hi there'); INSERT 0 1 postgres=# insert into foo values (2, 'bi there'); INSERT 0 1 postgres=# insert into foo values (2, 'error there') returning foo; ERROR: table row type and query-specified row type do not match DETAIL: Table has type text at ordinal position 1, but query expects integer. This is due to a mismatch between the composite type tuple descriptor of the leaf partition doesn't match the RETURNING slot tuple descriptor. We probably need to do what inheritance_planner()=>adjust_appendrel_attrs() does for adjusting the attrs in the child rel parse tree; i.e., handle some specific nodes other than just simple var nodes. In adjust_appendrel_attrs_mutator(), for whole row node, it updates var->vartype to the child rel. I suspect that the other nodes that adjust_appendrel_attrs_mutator handles (like JoinExpr, CurrentOfExpr, etc ) may also require similar adjustments for our case, because WithCheckOptions (and I think even RETURNING) can have a subquery. > > 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 > -- Thanks, -Amit Khandekar 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
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On Fri, Jul 28, 2017 at 1:06 AM, Noah Misch wrote: > [Action required within three days. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 10 open item. Robert, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. I'll try to get this resolved by the end of next week, but I don't know if that will be possible. I don't completely understand the issue yet. If we're remapping the varattnos, I don't see how we can just pass whole-row references through. I mean, if the partition and the parent have different varattnos, then a whole-row attribute for one is a different thing from a whole-row attribute for the other; the HeapTuple you would need to build in each case is different, based on the column order for the relation you're worrying about. (Boy, our implementation of DROP COLUMN is painful! If we really got rid of columns when they were dropped we could've avoided this whole mess.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
Robert Haas wrote: (Boy, our implementation of DROP COLUMN is painful! If we really got rid of columns when they were dropped we could've avoided this whole mess.) I tend to agree. I can recall several cases where it led to bugs that went undetected for quite a while. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
Robert Haas writes: > If we're remapping the varattnos, I don't see how we can just pass > whole-row references through. I mean, if the partition and the parent > have different varattnos, then a whole-row attribute for one is a > different thing from a whole-row attribute for the other; the > HeapTuple you would need to build in each case is different, based on > the column order for the relation you're worrying about. There is longstanding code in the planner to handle this for traditional- inheritance cases; IIRC what it does is build a ROW() expression that emits the proper output rowtype regardless of the discrepancies. Not sure why that's apparently not getting applied for partitioning. > (Boy, our implementation of DROP COLUMN is painful! If we really got > rid of columns when they were dropped we could've avoided this whole > mess.) I think the pain arises mostly from the decision to allow partitions to not all have identical rowtype. I would have lobbied against that choice if I'd been paying more attention at the start ... but I wasn't. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On Fri, Jul 28, 2017 at 3:26 PM, Tom Lane wrote: >> (Boy, our implementation of DROP COLUMN is painful! If we really got >> rid of columns when they were dropped we could've avoided this whole >> mess.) > > I think the pain arises mostly from the decision to allow partitions > to not all have identical rowtype. I would have lobbied against that > choice if I'd been paying more attention at the start ... but I wasn't. Given the way DROP COLUMN works, I can't really imagine that being a hit with end users even if you'd won the argument on this list. It would be totally reasonable to ask users to put the columns in the same order in all partitions, but asking them to have identically-sized and positioned dropped columns is full of fail. On the other hand, if DROP COLUMN didn't work this way, then you'd definitely have won that argument and we would be spared this bug (and many others). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
Thanks a lot Amit for looking at this and providing some useful pointers. On 2017/07/28 20:46, Amit Khandekar wrote: > On 28 July 2017 at 11:17, Amit Langote wrote: >> On 2017/07/26 16:58, Amit Langote wrote: >>> Rajkumar Raghuwanshi reported [1] on the "UPDATE partition key" thread >>> that whole-row vars don't play nicely with partitioned table operations. >>> >>> For example, when used to convert WITH CHECK OPTION constraint expressions >>> and RETURNING target list expressions, it will error out if the >>> expressions contained whole-row vars. That's a bug, because whole-row >>> vars are legal in those expressions. I think the decision to output error >>> upon encountering a whole-row in the input expression was based on the >>> assumption that it will only ever be used to convert partition constraint >>> expressions, which cannot contain those. So, we can relax that >>> requirement so that its other users are not bitten by it. >>> >>> Attached fixes that. >> >> Attached a revised version of the patch. >> >> Updated patch moves the if (found_whole_row) elog(ERROR) bit in >> map_partition_varattnos to the callers. Only the callers know if >> whole-row vars are not expected in the expression it's getting converted >> from map_partition_varattnos. Given the current message text (mentioning >> "partition key"), it didn't seem appropriate to have the elog inside this >> function, since it's callable from places where it wouldn't make sense. > > create table foo (a int, b text) partition by list (a); > create table foo1 partition of foo for values in (1); > create table foo2(b text, a int) ; > alter table foo attach partition foo2 for values in (2); > > postgres=# insert into foo values (1, 'hi there'); > INSERT 0 1 > postgres=# insert into foo values (2, 'bi there'); > INSERT 0 1 > postgres=# insert into foo values (2, 'error there') returning foo; > ERROR: table row type and query-specified row type do not match > DETAIL: Table has type text at ordinal position 1, but query expects integer. Didn't see that coming. > This is due to a mismatch between the composite type tuple descriptor > of the leaf partition doesn't match the RETURNING slot tuple > descriptor. > > We probably need to do what > inheritance_planner()=>adjust_appendrel_attrs() does for adjusting the > attrs in the child rel parse tree; i.e., handle some specific nodes > other than just simple var nodes. In adjust_appendrel_attrs_mutator(), > for whole row node, it updates var->vartype to the child rel. Yes, that's what's needed here. So we need to teach map_variable_attnos_mutator() to convert whole-row vars just like it's done in adjust_appendrel_attrs_mutator(). > I suspect that the other nodes that adjust_appendrel_attrs_mutator > handles (like JoinExpr, CurrentOfExpr, etc ) may also require similar > adjustments for our case, because WithCheckOptions (and I think even > RETURNING) can have a subquery. Actually, WITH CHECK and RETURNING expressions that are processed in the executor (i.e., in ExecInitModifyTable) are finished plan tree expressions (not parse or rewritten parse tree expressions), so we need not have to worry about having to convert those things in this case. Remember that adjust_appendrel_attrs_mutator() has to handle raw Query trees, because we plan the whole query separately for each partition in the UPDATE and DELETE (inheritance_planner). Since we don't need to plan an INSERT query for each partition separately (at least without the foreign tuple-routing support), we need not worry about converting anything beside Vars (with proper support for converting whole-row ones which you discovered has been missing), which we can do within the executor on the finished plan tree expressions. Attached 2 patches: 0001: Addresses the bug that Rajkumar reported (handling whole-row vars in WITH CHECK and RETURNING expressions at all) 0002: Addressed the bug that Amit reported (converting whole-row vars that could occur in WITH CHECK and RETURNING expressions) Thanks, Amit From d6b2169360d7951e229bb39ef53992edde70627b Mon Sep 17 00:00:00 2001 From: amit Date: Wed, 26 Jul 2017 16:45:46 +0900 Subject: [PATCH 1/2] Fix map_partition_varattnos to sometimes accept wholerow vars It was thought that it would never encount wholerow vars in its input expressions (partition constraint expressions for example). But then it was used to convert expressions where wholerow vars are legal, such as, WCO constraint expressions and RETURNING target list members. So, add an argument to tell it whether or not to error on finding wholerows. Adds test in insert.sql and updatable_views.sql. Reported by: Rajkumar Raghuwanshi Report: https://postgr.es/m/CAKcux6%3Dz38gH4K6YAFi%2BYvo5tHTwBL4tam4VM33CAPZ5dDMk1Q%40mail.gmail.com --- src/backend/catalog/partition.c | 17 ++--- src/backend/commands/tablecmds.c | 8 +++- src/backend/executor/nodeModifyTable.c| 18 ++
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/07/31 18:56, Amit Langote wrote: On 2017/07/28 20:46, Amit Khandekar wrote: create table foo (a int, b text) partition by list (a); create table foo1 partition of foo for values in (1); create table foo2(b text, a int) ; alter table foo attach partition foo2 for values in (2); postgres=# insert into foo values (1, 'hi there'); INSERT 0 1 postgres=# insert into foo values (2, 'bi there'); INSERT 0 1 postgres=# insert into foo values (2, 'error there') returning foo; ERROR: table row type and query-specified row type do not match DETAIL: Table has type text at ordinal position 1, but query expects integer. This is due to a mismatch between the composite type tuple descriptor of the leaf partition doesn't match the RETURNING slot tuple descriptor. We probably need to do what inheritance_planner()=>adjust_appendrel_attrs() does for adjusting the attrs in the child rel parse tree; i.e., handle some specific nodes other than just simple var nodes. In adjust_appendrel_attrs_mutator(), for whole row node, it updates var->vartype to the child rel. Yes, that's what's needed here. So we need to teach map_variable_attnos_mutator() to convert whole-row vars just like it's done in adjust_appendrel_attrs_mutator(). Seems reasonable. (Though I think it might be better to do this kind of conversion in the planner, not the executor, because that would increase the efficiency of cached plans.) I suspect that the other nodes that adjust_appendrel_attrs_mutator handles (like JoinExpr, CurrentOfExpr, etc ) may also require similar adjustments for our case, because WithCheckOptions (and I think even RETURNING) can have a subquery. Actually, WITH CHECK and RETURNING expressions that are processed in the executor (i.e., in ExecInitModifyTable) are finished plan tree expressions (not parse or rewritten parse tree expressions), so we need not have to worry about having to convert those things in this case. Remember that adjust_appendrel_attrs_mutator() has to handle raw Query trees, because we plan the whole query separately for each partition in the UPDATE and DELETE (inheritance_planner). Since we don't need to plan an INSERT query for each partition separately (at least without the foreign tuple-routing support), we need not worry about converting anything beside Vars (with proper support for converting whole-row ones which you discovered has been missing), which we can do within the executor on the finished plan tree expressions. Yeah, sublinks in WITH CHECK OPTION and RETURNING would already have been converted to subplans by the planner, so we don't need to worry about recursing into subqueries in sublinks at the execution time. Attached 2 patches: 0001: Addresses the bug that Rajkumar reported (handling whole-row vars in WITH CHECK and RETURNING expressions at all) 0002: Addressed the bug that Amit reported (converting whole-row vars that could occur in WITH CHECK and RETURNING expressions) I took a quick look at the patches. One thing I noticed is this: map_variable_attnos(Node *node, int target_varno, int sublevels_up, const AttrNumber *attno_map, int map_length, + Oid from_rowtype, Oid to_rowtype, bool *found_whole_row) { map_variable_attnos_context context; @@ -1291,6 +1318,8 @@ map_variable_attnos(Node *node, context.sublevels_up = sublevels_up; context.attno_map = attno_map; context.map_length = map_length; + context.from_rowtype = from_rowtype; + context.to_rowtype = to_rowtype; context.found_whole_row = found_whole_row; You added two arguments to pass to map_variable_attnos(): from_rowtype and to_rowtype. But I'm not sure that we really need the from_rowtype argument because it's possible to get the Oid from the vartype of target whole-row Vars. And in this: + /* +* If the callers expects us to convert the same, do so if +* necessary. +*/ + if (OidIsValid(context->to_rowtype) && + OidIsValid(context->from_rowtype) && + context->to_rowtype != context->from_rowtype) + { + ConvertRowtypeExpr *r = makeNode(ConvertRowtypeExpr); + + r->arg = (Expr *) newvar; + r->resulttype = context->from_rowtype; + r->convertformat = COERCE_IMPLICIT_CAST; + r->location = -1; + /* Make sure the Var node has the right type ID, too */ + newvar-
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 1 August 2017 at 15:11, Etsuro Fujita wrote: > On 2017/07/31 18:56, Amit Langote wrote: >> Yes, that's what's needed here. So we need to teach >> map_variable_attnos_mutator() to convert whole-row vars just like it's >> done in adjust_appendrel_attrs_mutator(). > > > Seems reasonable. (Though I think it might be better to do this kind of > conversion in the planner, not the executor, because that would increase the > efficiency of cached plans.) I think the work of shifting to planner should be taken as a different task when we shift the preparation of DispatchInfo to the planner. > >>> I suspect that the other nodes that adjust_appendrel_attrs_mutator >>> handles (like JoinExpr, CurrentOfExpr, etc ) may also require similar >>> adjustments for our case, because WithCheckOptions (and I think even >>> RETURNING) can have a subquery. >> >> >> Actually, WITH CHECK and RETURNING expressions that are processed in the >> executor (i.e., in ExecInitModifyTable) are finished plan tree expressions >> (not parse or rewritten parse tree expressions), so we need not have to >> worry about having to convert those things in this case. >> >> Remember that adjust_appendrel_attrs_mutator() has to handle raw Query >> trees, because we plan the whole query separately for each partition in >> the UPDATE and DELETE (inheritance_planner). Since we don't need to plan >> an INSERT query for each partition separately (at least without the >> foreign tuple-routing support), we need not worry about converting >> anything beside Vars (with proper support for converting whole-row ones >> which you discovered has been missing), which we can do within the >> executor on the finished plan tree expressions. > > > Yeah, sublinks in WITH CHECK OPTION and RETURNING would already have been > converted to subplans by the planner, so we don't need to worry about > recursing into subqueries in sublinks at the execution time. Yes, that seems true. It seems none of the nodes handled by adjust_appendrel_attrs_mutator() other than Var nodes exist in planned expressions. So looks like just additionally handling whole rows should be sufficient. > >> Attached 2 patches: >> >> 0001: Addresses the bug that Rajkumar reported (handling whole-row vars in >>WITH CHECK and RETURNING expressions at all) >> >> 0002: Addressed the bug that Amit reported (converting whole-row vars >>that could occur in WITH CHECK and RETURNING expressions) > > > I took a quick look at the patches. One thing I noticed is this: > > map_variable_attnos(Node *node, > int target_varno, int sublevels_up, > const AttrNumber *attno_map, int > map_length, > + Oid from_rowtype, Oid to_rowtype, > bool *found_whole_row) > { > map_variable_attnos_context context; > @@ -1291,6 +1318,8 @@ map_variable_attnos(Node *node, > context.sublevels_up = sublevels_up; > context.attno_map = attno_map; > context.map_length = map_length; > + context.from_rowtype = from_rowtype; > + context.to_rowtype = to_rowtype; > context.found_whole_row = found_whole_row; > > You added two arguments to pass to map_variable_attnos(): from_rowtype and > to_rowtype. But I'm not sure that we really need the from_rowtype argument > because it's possible to get the Oid from the vartype of target whole-row > Vars. And in this: > > + /* > +* If the callers expects us to convert the > same, do so if > +* necessary. > +*/ > + if (OidIsValid(context->to_rowtype) && > + OidIsValid(context->from_rowtype) && > + context->to_rowtype != > context->from_rowtype) > + { > + ConvertRowtypeExpr *r = > makeNode(ConvertRowtypeExpr); > + > + r->arg = (Expr *) newvar; > + r->resulttype = > context->from_rowtype; > + r->convertformat = > COERCE_IMPLICIT_CAST; > + r->location = -1; > + /* Make sure the Var node has the > right type ID, too */ > + newvar->vartype = > context->to_rowtype; > + return (Node *) r; > + } > > I think we could set r->resulttype to the vartype (ie, "r->resulttype = > newvar->vartype" instead of "r->resulttype = context->from_rowtype"). I agree. --- Few more comments : @@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node, var->varlevelsup == context->sublevels_up) { /* Found a matchin
Re: [HACKERS] map_partition_varattnos() and whole-row vars
Thanks Fuita-san and Amit for reviewing. On 2017/08/02 1:33, Amit Khandekar wrote: > On 1 August 2017 at 15:11, Etsuro Fujita wrote: >> On 2017/07/31 18:56, Amit Langote wrote: >>> Yes, that's what's needed here. So we need to teach >>> map_variable_attnos_mutator() to convert whole-row vars just like it's >>> done in adjust_appendrel_attrs_mutator(). >> >> >> Seems reasonable. (Though I think it might be better to do this kind of >> conversion in the planner, not the executor, because that would increase the >> efficiency of cached plans.) That's a good point, although it sounds like a bigger project that, IMHO, should be undertaken separately, because that would involve designing for considerations of expanding inheritance even in the INSERT case. > I think the work of shifting to planner should be taken as a different > task when we shift the preparation of DispatchInfo to the planner. Yeah, I think it'd be a good idea to do those projects together. That is, doing what Fujita-san suggested and expanding partitioned tables in partition bound order in the planner. >>> Attached 2 patches: >>> >>> 0001: Addresses the bug that Rajkumar reported (handling whole-row vars in >>>WITH CHECK and RETURNING expressions at all) >>> >>> 0002: Addressed the bug that Amit reported (converting whole-row vars >>>that could occur in WITH CHECK and RETURNING expressions) >> >> >> I took a quick look at the patches. One thing I noticed is this: >> >> map_variable_attnos(Node *node, >> int target_varno, int sublevels_up, >> const AttrNumber *attno_map, int >> map_length, >> + Oid from_rowtype, Oid to_rowtype, >> bool *found_whole_row) >> { >> map_variable_attnos_context context; >> @@ -1291,6 +1318,8 @@ map_variable_attnos(Node *node, >> context.sublevels_up = sublevels_up; >> context.attno_map = attno_map; >> context.map_length = map_length; >> + context.from_rowtype = from_rowtype; >> + context.to_rowtype = to_rowtype; >> context.found_whole_row = found_whole_row; >> >> You added two arguments to pass to map_variable_attnos(): from_rowtype and >> to_rowtype. But I'm not sure that we really need the from_rowtype argument >> because it's possible to get the Oid from the vartype of target whole-row >> Vars. And in this: >> >> + /* >> +* If the callers expects us to convert the >> same, do so if >> +* necessary. >> +*/ >> + if (OidIsValid(context->to_rowtype) && >> + OidIsValid(context->from_rowtype) && >> + context->to_rowtype != >> context->from_rowtype) >> + { >> + ConvertRowtypeExpr *r = >> makeNode(ConvertRowtypeExpr); >> + >> + r->arg = (Expr *) newvar; >> + r->resulttype = >> context->from_rowtype; >> + r->convertformat = >> COERCE_IMPLICIT_CAST; >> + r->location = -1; >> + /* Make sure the Var node has the >> right type ID, too */ >> + newvar->vartype = >> context->to_rowtype; >> + return (Node *) r; >> + } >> >> I think we could set r->resulttype to the vartype (ie, "r->resulttype = >> newvar->vartype" instead of "r->resulttype = context->from_rowtype"). > > I agree. You're right, from_rowtype is unnecessary. > --- > > Few more comments : > > @@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node, > var->varlevelsup == context->sublevels_up) > { > /* Found a matching variable, make the substitution */ > > - Var*newvar = (Var *) palloc(sizeof(Var)); > + Var*newvar = copyObject(var); > int attno = var->varattno; > > *newvar = *var; > > Here, "*newvar = *var" should be removed. Done. > --- > > - result = map_partition_varattnos(result, 1, rel, parent); > + result = map_partition_varattnos(result, 1, rel, parent, > + > &found_whole_row); > + /* There can never be a whole-row reference here */ > + if (found_whole_row) > + elog(ERROR, "unexpected whole-row reference found in > partition key"); > > Instead of callers of map_partition_varattnos() reporting error, we > can have map_partition_varattnos() itself report error. Instead of the > found_whole_row parameter of map_partition_varattnos(), we can have > error_on_whole_row parameter. So callers who don't expect whole row, > would p
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/08/02 15:21, Amit Langote wrote: > Thanks Fuita-san and Amit for reviewing. Sorry, I meant Fujita-san. - Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On Wed, Aug 2, 2017 at 2:25 AM, Amit Langote wrote: > On 2017/08/02 15:21, Amit Langote wrote: >> Thanks Fuita-san and Amit for reviewing. > > Sorry, I meant Fujita-san. Time is growing short here. Is there more review or coding that needs to be done here? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/08/03 12:06, Robert Haas wrote: On Wed, Aug 2, 2017 at 2:25 AM, Amit Langote wrote: On 2017/08/02 15:21, Amit Langote wrote: Thanks Fuita-san and Amit for reviewing. Sorry, I meant Fujita-san. Time is growing short here. Is there more review or coding that needs to be done here? I'll take another look at the patch from now. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2 August 2017 at 11:51, Amit Langote wrote: > Thanks Fuita-san and Amit for reviewing. > > On 2017/08/02 1:33, Amit Khandekar wrote: >> On 1 August 2017 at 15:11, Etsuro Fujita wrote: >>> On 2017/07/31 18:56, Amit Langote wrote: Yes, that's what's needed here. So we need to teach map_variable_attnos_mutator() to convert whole-row vars just like it's done in adjust_appendrel_attrs_mutator(). >>> >>> >>> Seems reasonable. (Though I think it might be better to do this kind of >>> conversion in the planner, not the executor, because that would increase the >>> efficiency of cached plans.) > > That's a good point, although it sounds like a bigger project that, IMHO, > should be undertaken separately, because that would involve designing for > considerations of expanding inheritance even in the INSERT case. > >> I think the work of shifting to planner should be taken as a different >> task when we shift the preparation of DispatchInfo to the planner. > > Yeah, I think it'd be a good idea to do those projects together. That is, > doing what Fujita-san suggested and expanding partitioned tables in > partition bound order in the planner. > Attached 2 patches: 0001: Addresses the bug that Rajkumar reported (handling whole-row vars in WITH CHECK and RETURNING expressions at all) 0002: Addressed the bug that Amit reported (converting whole-row vars that could occur in WITH CHECK and RETURNING expressions) >>> >>> >>> I took a quick look at the patches. One thing I noticed is this: >>> >>> map_variable_attnos(Node *node, >>> int target_varno, int sublevels_up, >>> const AttrNumber *attno_map, int >>> map_length, >>> + Oid from_rowtype, Oid to_rowtype, >>> bool *found_whole_row) >>> { >>> map_variable_attnos_context context; >>> @@ -1291,6 +1318,8 @@ map_variable_attnos(Node *node, >>> context.sublevels_up = sublevels_up; >>> context.attno_map = attno_map; >>> context.map_length = map_length; >>> + context.from_rowtype = from_rowtype; >>> + context.to_rowtype = to_rowtype; >>> context.found_whole_row = found_whole_row; >>> >>> You added two arguments to pass to map_variable_attnos(): from_rowtype and >>> to_rowtype. But I'm not sure that we really need the from_rowtype argument >>> because it's possible to get the Oid from the vartype of target whole-row >>> Vars. And in this: >>> >>> + /* >>> +* If the callers expects us to convert the >>> same, do so if >>> +* necessary. >>> +*/ >>> + if (OidIsValid(context->to_rowtype) && >>> + OidIsValid(context->from_rowtype) && >>> + context->to_rowtype != >>> context->from_rowtype) >>> + { >>> + ConvertRowtypeExpr *r = >>> makeNode(ConvertRowtypeExpr); >>> + >>> + r->arg = (Expr *) newvar; >>> + r->resulttype = >>> context->from_rowtype; >>> + r->convertformat = >>> COERCE_IMPLICIT_CAST; >>> + r->location = -1; >>> + /* Make sure the Var node has the >>> right type ID, too */ >>> + newvar->vartype = >>> context->to_rowtype; >>> + return (Node *) r; >>> + } >>> >>> I think we could set r->resulttype to the vartype (ie, "r->resulttype = >>> newvar->vartype" instead of "r->resulttype = context->from_rowtype"). >> >> I agree. > > You're right, from_rowtype is unnecessary. > >> --- >> >> Few more comments : >> >> @@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node, >> var->varlevelsup == context->sublevels_up) >> { >> /* Found a matching variable, make the substitution */ >> >> - Var*newvar = (Var *) palloc(sizeof(Var)); >> + Var*newvar = copyObject(var); >> int attno = var->varattno; >> >> *newvar = *var; >> >> Here, "*newvar = *var" should be removed. > > Done. > >> --- >> >> - result = map_partition_varattnos(result, 1, rel, parent); >> + result = map_partition_varattnos(result, 1, rel, parent, >> + >> &found_whole_row); >> + /* There can never be a whole-row reference here */ >> + if (found_whole_row) >> + elog(ERROR, "unexpected whole-row reference found in >> partition key"); >> >> Instead of callers of map_partition_varattnos() reporting error, we >> can have map_partition_varattnos() itself report
Re: [HACKERS] map_partition_varattnos() and whole-row vars
Thanks for the review. On 2017/08/03 13:54, Amit Khandekar wrote: > On 2 August 2017 at 11:51, Amit Langote wrote: >> On 2017/08/02 1:33, Amit Khandekar wrote: >>> Instead of callers of map_partition_varattnos() reporting error, we >>> can have map_partition_varattnos() itself report error. Instead of the >>> found_whole_row parameter of map_partition_varattnos(), we can have >>> error_on_whole_row parameter. So callers who don't expect whole row, >>> would pass error_on_whole_row=true to map_partition_varattnos(). This >>> will simplify the resultant code a bit. >> >> So, I think it's only the callers of >> map_partition_varattnos() who know whether finding whole-row vars is an >> error and *what kind* of error. > > Ok. Got it. Thanks for the explanation. > > How about making found_whole_row as an optional parameter ? > map_partition_varattnos() would populate it only if its not NULL. This > way, callers who don't bother about whether it is found or not don't > have to declare a variable and pass its address. In current code, > ExecInitModifyTable() is the one who has to declare found_whole_row > without needing it. Sure, done. But while implementing that, I avoided changing anything in map_variable_attnos(), such as adding a check for not-NULLness of found_whole_row. The only check added is in map_partition_varattnos(). Attached updated patches. Thanks, Amit From 00a46ef3cfac3b3b197ccf39b8749f06d0d942ad Mon Sep 17 00:00:00 2001 From: amit Date: Wed, 26 Jul 2017 16:45:46 +0900 Subject: [PATCH 1/2] Fix map_partition_varattnos to not error on found_whole_row It was designed assuming that the expressions passed to it can never contain whole-row vars, but it's wrong since it's called from places that pass it WCO constraint expressions and RETURNING target list expressions, which can very well contain whole-row vars. Move the responsibility of error'ing out to the callers, because they are in position to know that finding whole-row vars is an error condition. Adds test in insert.sql and updatable_views.sql. Reported by: Rajkumar Raghuwanshi Report: https://postgr.es/m/CAKcux6%3Dz38gH4K6YAFi%2BYvo5tHTwBL4tam4VM33CAPZ5dDMk1Q%40mail.gmail.com --- src/backend/catalog/partition.c | 17 +++-- src/backend/commands/tablecmds.c | 8 +++- src/backend/executor/nodeModifyTable.c| 14 ++ src/include/catalog/partition.h | 3 ++- src/test/regress/expected/insert.out | 10 ++ src/test/regress/expected/updatable_views.out | 10 ++ src/test/regress/sql/insert.sql | 6 ++ src/test/regress/sql/updatable_views.sql | 9 + 8 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 9d50efb6a0..5891af9cf2 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -904,10 +904,11 @@ get_qual_from_partbound(Relation rel, Relation parent, */ List * map_partition_varattnos(List *expr, int target_varno, - Relation partrel, Relation parent) + Relation partrel, Relation parent, + bool *found_whole_row) { AttrNumber *part_attnos; - boolfound_whole_row; + boolmy_found_whole_row; if (expr == NIL) return NIL; @@ -919,10 +920,9 @@ map_partition_varattnos(List *expr, int target_varno, target_varno, 0, part_attnos, RelationGetDescr(parent)->natts, - &found_whole_row); - /* There can never be a whole-row reference here */ + &my_found_whole_row); if (found_whole_row) - elog(ERROR, "unexpected whole-row reference found in partition key"); + *found_whole_row = my_found_whole_row; return expr; } @@ -1783,6 +1783,7 @@ generate_partition_qual(Relation rel) List *my_qual = NIL, *result = NIL; Relationparent; + boolfound_whole_row; /* Guard against stack overflow due to overly deep partition tree */ check_stack_depth(); @@ -1825,7 +1826,11 @@ generate_partition_qual(Relation rel) * in it to bear this relation's attnos. It's safe to assume varno = 1 * here. */ - result = map_partition_varattnos(result, 1, rel, parent); + result = map_partition_varattnos(result, 1, rel, parent, +
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 3 August 2017 at 11:00, Amit Langote wrote: > Thanks for the review. > > On 2017/08/03 13:54, Amit Khandekar wrote: >> On 2 August 2017 at 11:51, Amit Langote wrote: >>> On 2017/08/02 1:33, Amit Khandekar wrote: Instead of callers of map_partition_varattnos() reporting error, we can have map_partition_varattnos() itself report error. Instead of the found_whole_row parameter of map_partition_varattnos(), we can have error_on_whole_row parameter. So callers who don't expect whole row, would pass error_on_whole_row=true to map_partition_varattnos(). This will simplify the resultant code a bit. >>> >>> So, I think it's only the callers of >>> map_partition_varattnos() who know whether finding whole-row vars is an >>> error and *what kind* of error. >> >> Ok. Got it. Thanks for the explanation. >> >> How about making found_whole_row as an optional parameter ? >> map_partition_varattnos() would populate it only if its not NULL. This >> way, callers who don't bother about whether it is found or not don't >> have to declare a variable and pass its address. In current code, >> ExecInitModifyTable() is the one who has to declare found_whole_row >> without needing it. > > Sure, done. Looks good. > But while implementing that, I avoided changing anything in > map_variable_attnos(), such as adding a check for not-NULLness of > found_whole_row. The only check added is in map_partition_varattnos(). Yes, I agree. That is anyway not relevant to the fix. -- Thanks, -Amit Khandekar 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
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/08/02 15:21, Amit Langote wrote: Thanks Fuita-san and Amit for reviewing. On 2017/08/02 1:33, Amit Khandekar wrote: On 1 August 2017 at 15:11, Etsuro Fujita wrote: On 2017/07/31 18:56, Amit Langote wrote: Yes, that's what's needed here. So we need to teach map_variable_attnos_mutator() to convert whole-row vars just like it's done in adjust_appendrel_attrs_mutator(). Seems reasonable. (Though I think it might be better to do this kind of conversion in the planner, not the executor, because that would increase the efficiency of cached plans.) That's a good point, although it sounds like a bigger project that, IMHO, should be undertaken separately, because that would involve designing for considerations of expanding inheritance even in the INSERT case. Agreed. I think that would be definitely a material for PG11. I think the work of shifting to planner should be taken as a different task when we shift the preparation of DispatchInfo to the planner. Yeah, I think it'd be a good idea to do those projects together. That is, doing what Fujita-san suggested and expanding partitioned tables in partition bound order in the planner. OK --- Few more comments : @@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node, var->varlevelsup == context->sublevels_up) { /* Found a matching variable, make the substitution */ - Var*newvar = (Var *) palloc(sizeof(Var)); + Var*newvar = copyObject(var); int attno = var->varattno; *newvar = *var; Here, "*newvar = *var" should be removed. Done. I'm not sure this change is a good idea, because the copy by "*newvar = *var" would be more efficient than the copyObject(). (We have this optimization in other places as well. See eg, copyVar() in setrefs.c.) Here are some other comments: + /* If the callers expects us to convert the same, do so. */ + if (OidIsValid(context->to_rowtype)) + { + ConvertRowtypeExpr *r; + + /* Var itself is converted to the requested rowtype. */ + newvar->vartype = context->to_rowtype; + + /* +* And a conversion step on top to convert back to the +* original type. +*/ + r = makeNode(ConvertRowtypeExpr); + r->arg = (Expr *) newvar; + r->resulttype = var->vartype; + r->convertformat = COERCE_IMPLICIT_CAST; + r->location = -1; + + return (Node *) r; + } Why not do this conversion if to_rowtype is valid and it's different from the rowtype of the original whole-row Var like the previous patch? Also, I think it's better to add an assertion that the rowtype of the original whole-row Var is a named one. So, what I have in mind is: if (OidIsValid(context->to_rowtype)) { Assert(var->vartype != RECORDOID) if (var->vartype != context->to_rowtype) { ConvertRowtypeExpr *r; /* Var itself is converted to the requested rowtype. */ ... /* And a conversion step on top to convert back to the ... */ ... return (Node *) r; } } Here is the modification to the map_variable_attnos()'s API: map_variable_attnos(Node *node, int target_varno, int sublevels_up, const AttrNumber *attno_map, int map_length, - bool *found_whole_row) + bool *found_whole_row, Oid to_rowtype) This is nitpicking, but I think it would be better to put the new argument to_rowtype right before the output argument found_whole_row. + * RelationGetRelType + * Returns the rel's pg_type OID. + */ +#define RelationGetRelType(relation) \ + ((relation)->rd_rel->reltype) This macro is used in only one place. Do we really need that? (This macro would be useful for other places such as expand_inherited_rtentry, but I think it's better to introduce this in a separate patch, maybe for PG11.) +-- check that wholerow vars in the RETUNING list work with partitioned tables Typo: s/RETUNING/RETURNING/ Sorry for the delay. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
Fujita-san, Thanks for the review. On 2017/08/03 16:01, Etsuro Fujita wrote: > On 2017/08/02 15:21, Amit Langote wrote: >> On 2017/08/02 1:33, Amit Khandekar wrote: >>> --- >>> >>> Few more comments : >>> >>> @@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node, >>> var->varlevelsup == context->sublevels_up) >>> { >>> /* Found a matching variable, make the substitution */ >>> >>> - Var*newvar = (Var *) palloc(sizeof(Var)); >>> + Var*newvar = copyObject(var); >>>int attno = var->varattno; >>> >>> *newvar = *var; >>> >>> Here, "*newvar = *var" should be removed. >> >> Done. > > I'm not sure this change is a good idea, because the copy by "*newvar = > *var" would be more efficient than the copyObject(). (We have this > optimization in other places as well. See eg, copyVar() in setrefs.c.) OK, done. > Here are some other comments: > > +/* If the callers expects us to convert the same, do so. */ > +if (OidIsValid(context->to_rowtype)) > +{ > +ConvertRowtypeExpr *r; > + > +/* Var itself is converted to the requested rowtype. */ > +newvar->vartype = context->to_rowtype; > + > +/* > + * And a conversion step on top to convert back to the > + * original type. > + */ > +r = makeNode(ConvertRowtypeExpr); > +r->arg = (Expr *) newvar; > +r->resulttype = var->vartype; > +r->convertformat = COERCE_IMPLICIT_CAST; > +r->location = -1; > + > +return (Node *) r; > +} > > Why not do this conversion if to_rowtype is valid and it's different from > the rowtype of the original whole-row Var like the previous patch? Also, I > think it's better to add an assertion that the rowtype of the original > whole-row Var is a named one. So, what I have in mind is: > > if (OidIsValid(context->to_rowtype)) > { > Assert(var->vartype != RECORDOID) > if (var->vartype != context->to_rowtype) > { > ConvertRowtypeExpr *r; > > /* Var itself is converted to the requested rowtype. */ > ... > /* And a conversion step on top to convert back to the ... */ > ... > return (Node *) r; > } > } Sounds good, so done. > Here is the modification to the map_variable_attnos()'s API: > > map_variable_attnos(Node *node, > int target_varno, int sublevels_up, > const AttrNumber *attno_map, int > map_length, > - bool *found_whole_row) > + bool *found_whole_row, Oid > to_rowtype) > > This is nitpicking, but I think it would be better to put the new argument > to_rowtype right before the output argument found_whole_row. I consider this a good suggestion. I guess we tend to list all the input arguments before any output arguments. So done as you suggest. > + * RelationGetRelType > + *Returns the rel's pg_type OID. > + */ > +#define RelationGetRelType(relation) \ > +((relation)->rd_rel->reltype) > > This macro is used in only one place. Do we really need that? (This > macro would be useful for other places such as expand_inherited_rtentry, > but I think it's better to introduce this in a separate patch, maybe for > PG11.) Alright, dropped RelationGetRelType from the patch. > +-- check that wholerow vars in the RETUNING list work with partitioned > tables > > Typo: s/RETUNING/RETURNING/ Fixed. Attached updated patches. Thanks, Amit From 9b2d16ec4c8eadd7261849d5aa0f34ee2577b405 Mon Sep 17 00:00:00 2001 From: amit Date: Wed, 26 Jul 2017 16:45:46 +0900 Subject: [PATCH 1/2] Fix map_partition_varattnos to not error on found_whole_row It was designed assuming that the expressions passed to it can never contain whole-row vars, but it's wrong since it's called from places that pass it WCO constraint expressions and RETURNING target list expressions, which can very well contain whole-row vars. Move the responsibility of error'ing out to the callers, because they are in position to know that finding whole-row vars is an error condition. Adds test in insert.sql and updatable_views.sql. Reported by: Rajkumar Raghuwanshi Report: https://postgr.es/m/CAKcux6%3Dz38gH4K6YAFi%2BYvo5tHTwBL4tam4VM33CAPZ5dDMk1Q%40mail.gmail.com --- src/backend/catalog/partition.c | 20 ++-- src/backend/commands/tablecmds.c | 8 +++- src/backend/executor/nodeModifyTable.c| 18 ++ src/include/catalog/partition.h | 3 ++- src/test/regress/expected/insert.out | 10 ++ src/test/regress/expected/updatable_views.out | 10 ++ src/test/regress/sql/insert.sql
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/08/03 17:12, Amit Langote wrote: Attached updated patches. Thanks for the patch! That looks good to me. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On Thu, Aug 3, 2017 at 4:40 AM, Etsuro Fujita wrote: > On 2017/08/03 17:12, Amit Langote wrote: >> Attached updated patches. > > Thanks for the patch! That looks good to me. Committed with some comment changes. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/08/04 1:06, Robert Haas wrote: > On Thu, Aug 3, 2017 at 4:40 AM, Etsuro Fujita > wrote: >> On 2017/08/03 17:12, Amit Langote wrote: >>> Attached updated patches. >> >> Thanks for the patch! That looks good to me. > > Committed with some comment changes. Thanks a lot. 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
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/08/04 10:00, Amit Langote wrote: On 2017/08/04 1:06, Robert Haas wrote: On Thu, Aug 3, 2017 at 4:40 AM, Etsuro Fujita wrote: On 2017/08/03 17:12, Amit Langote wrote: Attached updated patches. Thanks for the patch! That looks good to me. Committed with some comment changes. Thanks a lot. Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers