Re: [HACKERS] pgsql: Avoid coercing a whole-row variable that is already coerced

2017-10-13 Thread Robert Haas 
On Fri, Oct 13, 2017 at 12:56 PM, Tom Lane  wrote:
> It's better ... but after reading the patched code, a lot of my remaining
> beef is with the lack of clarity of the comments.  You need ESP to
> understand what the function is trying to accomplish and what the
> constraints are.  I'll take a whack at improving that and push.

OK, thanks.   The good thing is, now that we know you have ESP, you
can use it with the Ouija board Andres used to decide whether to apply
sizeof to the variable or the type.  That's got to be a win.

-- 
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] pgsql: Avoid coercing a whole-row variable that is already coerced

2017-10-13 Thread Tom Lane 
Robert Haas  writes:
> On Fri, Oct 13, 2017 at 5:57 AM, Amit Khandekar  
> wrote:
>> One thing we can do is : instead of calling
>> map_variable_attnos_mutator(), convert the var inside the if block for
>> "if (IsA(node, ConvertRowtypeExpr))". Please check the attached patch.
>> There, I have avoided coerced_var context variable.

> Tom, is this more like what you have in mind?

It's better ... but after reading the patched code, a lot of my remaining
beef is with the lack of clarity of the comments.  You need ESP to
understand what the function is trying to accomplish and what the
constraints are.  I'll take a whack at improving that and push.

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] pgsql: Avoid coercing a whole-row variable that is already coerced

2017-10-13 Thread Robert Haas 
On Fri, Oct 13, 2017 at 5:57 AM, Amit Khandekar  wrote:
> One thing we can do is : instead of calling
> map_variable_attnos_mutator(), convert the var inside the if block for
> "if (IsA(node, ConvertRowtypeExpr))". Please check the attached patch.
> There, I have avoided coerced_var context variable.

Tom, is this more like what you have in mind?

-- 
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] pgsql: Avoid coercing a whole-row variable that is already coerced

2017-10-13 Thread Amit Khandekar 
Bringing here the mail thread from pgsql-committers  regarding this commit :

commit 1c497fa72df7593d8976653538da3d0ab033207f
Author: Robert Haas 
Date:   Thu Oct 12 17:10:48 2017 -0400

Avoid coercing a whole-row variable that is already coerced.

Marginal efficiency and beautification hack.  I'm not sure whether
this case ever arises currently, but the pending patch for update
tuple routing will cause it to arise.

Amit Khandekar

Discussion:
http://postgr.es/m/caj3gd9cazfppe7-wwubabpcq4_0subkipfd1+0r5_dkvnwo...@mail.gmail.com


Tom Lane  wrote:
> Robert Haas  wrote:
> > Avoid coercing a whole-row variable that is already coerced.
>
> This logic seems very strange and more than likely buggy: the
> added coerced_var flag feels like the sort of action at a distance
> that is likely to have unforeseen side effects.
>
> I'm not entirely sure what the issue is here, but if you're concerned
> about not applying two ConvertRowtypeExprs in a row, why not have the
> upper one just strip off the lower one?  We handle, eg, nested
> RelabelTypes that way.

We kind of do a similar thing. When a ConvertRowtypeExpr node is
encountered, we create a new ConvertRowtypeExpr that points to a new
var, and return this new ConvertRowtypeExpr instead of the existing
one. So we actually replace the old with a new one. But additionally,
we also want to change the vartype of the new var to
context->to_rowtype.

I think you are worried specifically about coerced_var causing
unexpected regression in existing scenarios, such as :
context->coerced_var getting set and prematurely unset in recursive
scenarios. But note that, when we call map_variable_attnos_mutator()
just after setting context->coerced_var = true,
map_variable_attnos_mutator() won't recurse further, because it is
always called with a Var, which does not have any further arguments to
process. So coerced_var won't be again changed until we return from
map_variable_attnos_mutator().

The only reason why we chose to call map_variable_attnos_mutator()
with a Var is so that we can re-use the code that converts the whole
row var.

One thing we can do is : instead of calling
map_variable_attnos_mutator(), convert the var inside the if block for
"if (IsA(node, ConvertRowtypeExpr))". Please check the attached patch.
There, I have avoided coerced_var context variable.


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company


handle-redundant-ConvertRowtypeExpr-node.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers