Re: Optimizing nested ConvertRowtypeExpr execution

At Mon, 9 Apr 2018 16:43:22 +0530, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote in <CAFjFpRcPaLM6AQf43QaYm-e3-8=o9QxsxahSMNqVvpDiWQfg_g(at)mail(dot)gmail(dot)com>
> On Mon, Apr 9, 2018 at 4:29 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > At Mon, 9 Apr 2018 16:07:33 +0530, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote in <CAFjFpRcCjqSdT70EQKUoCkfinh3VOwqZDZbg6YROzw_M0Pwd+g(at)mail(dot)gmail(dot)com>
> >> On Mon, Apr 9, 2018 at 3:53 PM, Ashutosh Bapat
> >> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> >> > On Mon, Apr 9, 2018 at 3:49 PM, Kyotaro HORIGUCHI
> >> > <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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