Thread: [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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 28 July 2017 at 11:17, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
On Fri, Jul 28, 2017 at 1:06 AM, Noah Misch <noah@leadboat.com> 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
Robert Haas <robertmhaas@gmail.com> 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
Robert Haas <robertmhaas@gmail.com> 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
On Fri, Jul 28, 2017 at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
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 <Langote_Amit_f8@lab.ntt.co.jp> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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_contextcontext;
@@ -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").
Best regards,
Etsuro Fujita
On 1 August 2017 at 15:11, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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
matchingvariable, 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.
-------
- 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 pass error_on_whole_row=true to map_partition_varattnos(). This
will simplify the resultant code a bit.
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
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 <fujita.etsuro@lab.ntt.co.jp> 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 pass error_on_whole_row=true to map_partition_varattnos(). This
> will simplify the resultant code a bit.
Actually, the first patch I posted on this thread did exactly the same
thing (it added a wholerow_ok argument to map_partition_varattnos similar
in spirit to your error_on_whole_row), but later dropped that idea because
of the ambiguity I felt about the error message text in
map_partition_varattnos().
Actually, unlike other callers of map_variable_attnos(),
map_partition_varattnos *can* in fact convert the whole-row Vars, because
it has all the available information to do so (some of the other callers
don't). It's just that some callers of map_partition_varattnos() convert
expressions containing only the partition key Vars which cannot be
whole-row Vars, so any whole-row Var that map_variable_attnos() finds is
an unexpected error condition. So the current text reads: "unexpected
whole-row reference found in partition key". There are other callers of
map_partition_varattnos() (more will crop up in the future) that don't
necessarily pass expressions containing only the partition key, so having
the above error message sounds odd. 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. I also think this reasoning is similar to
why map_variable_attnos_mutator() itself does not output error on seeing a
whole-row var.
Attached updated patches.
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
Attachment
On 2017/08/02 15:21, Amit Langote wrote: > Thanks Fuita-san and Amit for reviewing. Sorry, I meant Fujita-san. - Amit
On Wed, Aug 2, 2017 at 2:25 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
On 2017/08/03 12:06, Robert Haas wrote: > On Wed, Aug 2, 2017 at 2:25 AM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> 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
On 2 August 2017 at 11:51, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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 <fujita.etsuro@lab.ntt.co.jp> 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 pass error_on_whole_row=true to map_partition_varattnos(). This
>> will simplify the resultant code a bit.
>
> Actually, the first patch I posted on this thread did exactly the same
> thing (it added a wholerow_ok argument to map_partition_varattnos similar
> in spirit to your error_on_whole_row), but later dropped that idea because
> of the ambiguity I felt about the error message text in
> map_partition_varattnos().
>
> Actually, unlike other callers of map_variable_attnos(),
> map_partition_varattnos *can* in fact convert the whole-row Vars, because
> it has all the available information to do so (some of the other callers
> don't). It's just that some callers of map_partition_varattnos() convert
> expressions containing only the partition key Vars which cannot be
> whole-row Vars, so any whole-row Var that map_variable_attnos() finds is
> an unexpected error condition. So the current text reads: "unexpected
> whole-row reference found in partition key". There are other callers of
> map_partition_varattnos() (more will crop up in the future) that don't
> necessarily pass expressions containing only the partition key, so having
> the above error message sounds odd. 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.
Other than this, with the updated patches, I have no more comments from my end.
> I also think this reasoning is similar to
> why map_variable_attnos_mutator() itself does not output error on seeing a
> whole-row var.
>
> Attached updated patches.
>
> Thanks,
> Amit
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 3 August 2017 at 11:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
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 <fujita.etsuro@lab.ntt.co.jp> 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
backto 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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
On Thu, Aug 3, 2017 at 4:40 AM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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
On 2017/08/04 1:06, Robert Haas wrote: > On Thu, Aug 3, 2017 at 4:40 AM, Etsuro Fujita > <fujita.etsuro@lab.ntt.co.jp> 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
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 >> <fujita.etsuro@lab.ntt.co.jp> 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