Horton

Thread

Commits

  1. Improve implementation of CRE-stack-flattening in map_variable_attnos().

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

  1. Re: pgsql: Avoid coercing a whole-row variable that is already coerced

    Amit Khandekar <amitdkhan.pg@gmail.com> — 2017-10-13T09:57:12Z

    Bringing here the mail thread from pgsql-committers  regarding this commit :

commit 1c497fa72df7593d8976653538da3d0ab033207f
Author: Robert Haas <rhaas@postgresql.org>
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_DkVNWo5yg@mail.gmail.com


Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <rhaas(at)postgresql(dot)org> 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

  2. Re: pgsql: Avoid coercing a whole-row variable that is already coerced

    Robert Haas <robertmhaas@gmail.com> — 2017-10-13T16:21:13Z

    On Fri, Oct 13, 2017 at 5:57 AM, Amit Khandekar <amitdkhan.pg@gmail.com> 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

  3. Re: pgsql: Avoid coercing a whole-row variable that is already coerced

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-10-13T16:56:31Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Oct 13, 2017 at 5:57 AM, Amit Khandekar <amitdkhan.pg@gmail.com> 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

  4. Re: pgsql: Avoid coercing a whole-row variable that is already coerced

    Robert Haas <robertmhaas@gmail.com> — 2017-10-13T17:11:28Z

    On Fri, Oct 13, 2017 at 12:56 PM, Tom Lane <tgl@sss.pgh.pa.us> 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