Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Etsuro Fujita]

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

Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Amit Langote]

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

[Robert Haas]

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

[Etsuro Fujita]

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

[Amit Langote]

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 ++
 

Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Etsuro Fujita]

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

[Amit Khandekar]

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

[Amit Langote]

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,
-   
_whole_row);
-   /* There can never be a whole-row reference here */
+   
_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

[Amit Khandekar]

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,
>> +
>>   _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, 

Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Etsuro Fujita]

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

[Robert Haas]

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

[Amit Langote]

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

[Amit Langote]

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,
> +
>   _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 

Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Amit Khandekar]

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 == 

Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Etsuro Fujita]

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 */
+   

Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Amit Langote]

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 +++-
 

Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Robert Haas]

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

[Tom Lane]

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

[Peter Geoghegan]

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]

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

[Amit Khandekar]

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

[Amit Langote]

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,
-   
_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.
 */
-   

Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Noah Misch]

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

[HACKERS] map_partition_varattnos() and whole-row vars

[Amit Langote]

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,

_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;