Re: Optimizing nested ConvertRowtypeExpr execution - Mailing list pgsql-hackers

At Mon, 9 Apr 2018 16:43:22 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in
<CAFjFpRcPaLM6AQf43QaYm-e3-8=o9QxsxahSMNqVvpDiWQfg_g@mail.gmail.com>
> On Mon, Apr 9, 2018 at 4:29 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > At Mon, 9 Apr 2018 16:07:33 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in
<CAFjFpRcCjqSdT70EQKUoCkfinh3VOwqZDZbg6YROzw_M0Pwd+g@mail.gmail.com>
> >> On Mon, Apr 9, 2018 at 3:53 PM, Ashutosh Bapat
> >> <ashutosh.bapat@enterprisedb.com> wrote:
> >> > On Mon, Apr 9, 2018 at 3:49 PM, Kyotaro HORIGUCHI
> >> > <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >> >>
> >> >> I don't think it is not only on constatns.  With the patch,
> >> >> non-constants are .. getting a rather strange conversion.
> >> >>
> >> >>
> >> >>> =# explain verbose select (a, b, c)::t1p1p1::t1p1::t1 from (select i, i * 2, i * 3 from generate_series(0, 10)
i)x(a, b, c);
 
> >> >>>                              QUERY PLAN
> >> >>> -------------------------------------------------------------------------
> >> >> ...
> >> >>>    Output: (ROW(i.i, (i.i * 2), (i.i * 3))::t1p1p1)::t1
> >> >>
> >> >> Conversions between scalar values cannot be assumed safely
> >> >> composed each other for general inputs but it is known to be safe
> >> >> for the ConvertRowtypeExpr case.. I think.
> >> >
> >> > I am not able to parse this statement.
> >> >
> >> > What output do you see without the patch?
> >> >
> >>
> >> Without the patch, I see
> >> explain verbose select (a, b, c)::t1p1p1::t1p1::t1 from (select i, i *
> >> 2, i * 3 from generate_series(0, 10) i) x(a, b, c);
> >>                                       QUERY PLAN
> >> --------------------------------------------------------------------------------------
> >>  Function Scan on pg_catalog.generate_series i  (cost=0.00..15.00
> >> rows=1000 width=32)
> >>    Output: ((ROW(i.i, (i.i * 2), (i.i * 3))::t1p1p1)::t1p1)::t1
> >>    Function Call: generate_series(0, 10)
> >> (3 rows)
> >>
> >> Only difference between the two outputs is direct conversion of t1p1p1
> >> row into t1 row, which is what is expected with this patch. Are you
> >> suggesting that the optimization attempted in the patch is not safe?
> >> If yes, can you please explain why, and give a scenario showing its
> >> "un"safety?
> >
> > I understood the reason for the current output. Maybe I meant the
> > contrary, we can remove intermediate conversions more
> > aggressively. I assumed that ::t1p1p1 and ::t1 yield the same
> > output from any input. Is it right?
> 
> We can't directly cast Row() into t1 unless it's compatible with a
> leaf child row hence ROW()::t1p1p1 cast. But I think that's a single
> expression not two as it looks.

I misunderstood that ConvertRowtypeExpr is only for partitioned
tables. I noticed that it is shared with inheritance. So it works
on inheritacne as the follows. RowExpr make it work differently.

> =# explain verbose select (1, 2, 3, 4, 5)::jt1p1p1::jt1p1::jt1; --
...
>    Output: '(1,2,3)'::jt1

Thanks for replaying patiently.

The new code doesn't seem to work as written.

>   arg = eval_const_expressions_mutator((Node *) cre->arg,
>                                        context);
> 
>   /*
>    * In case of a nested ConvertRowtypeExpr, we can convert the
>    * leaf row directly to the topmost row format without any
>    * intermediate conversions.
>    */
>   while (arg != NULL && IsA(arg, ConvertRowtypeExpr))
>       arg = (Node *) castNode(ConvertRowtypeExpr, arg)->arg;

This runs depth-first so the while loop seems to run at most
once. I suppose that the "arg =" and the while loop are
transposed as intention.

>  arg = (Node *) cre->arg;
>  while (arg != NULL && IsA(arg, ConvertRowtypeExpr))
>      arg = (Node *) castNode(ConvertRowtypeExpr, arg)->arg;
> 
>  arg = eval_const_expressions_mutator(arg, context);

It looks fine except the above point.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center