Обсуждение: Optimizing nested ConvertRowtypeExpr execution
Hi, In a multi-level partitioned table, a parent whole-row reference gets translated into nested ConvertRowtypeExpr with child whole-row reference as the leaf. During the execution, the child whole-row reference gets translated into all all intermediate parents' whole-row references, ultimately represented as parent's whole-row reference. AFAIU, the intermediate translations are unnecessary. The leaf child whole-row can be directly translated into top parent's whole-row reference. Here's a WIP patch which does that by eliminating intermediate ConvertRowtypeExprs during ExecInitExprRec(). I tested the performance with two-level partitions, and 1M rows, on my laptop selecting just the whole-row expression. I saw ~20% improvement in the execution time. Please see the attached test and its output with and without patch. For two-level inheritance hierarchy, this patch doesn't show any performance difference since the plan time hierarchy is flattened into single level. Instead of the approach that the patch takes, we might modify adjust_appendrel_attrs() not to produce nested ConvertRowtypeExprs in the first place. With that we may get rid of code which handles nested ConvertRowtypeExprs like is_converted_whole_row_reference(). But I haven't tried that approach yet. Suggestions/comments welcome. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Вложения
Hi, On 2018-02-26 17:20:05 +0530, Ashutosh Bapat wrote: > In a multi-level partitioned table, a parent whole-row reference gets > translated into nested ConvertRowtypeExpr with child whole-row > reference as the leaf. During the execution, the child whole-row > reference gets translated into all all intermediate parents' whole-row > references, ultimately represented as parent's whole-row reference. > AFAIU, the intermediate translations are unnecessary. The leaf child > whole-row can be directly translated into top parent's whole-row > reference. Here's a WIP patch which does that by eliminating > intermediate ConvertRowtypeExprs during ExecInitExprRec(). Why is this done appropriately at ExecInitExpr() time, rather than at plan time? Seems like eval_const_expressions() would be a bit more appropriate (being badly named aside...)? - Andres
On Mon, Apr 2, 2018 at 1:40 AM, Andres Freund <andres@anarazel.de> wrote: > Hi, > > On 2018-02-26 17:20:05 +0530, Ashutosh Bapat wrote: >> In a multi-level partitioned table, a parent whole-row reference gets >> translated into nested ConvertRowtypeExpr with child whole-row >> reference as the leaf. During the execution, the child whole-row >> reference gets translated into all all intermediate parents' whole-row >> references, ultimately represented as parent's whole-row reference. >> AFAIU, the intermediate translations are unnecessary. The leaf child >> whole-row can be directly translated into top parent's whole-row >> reference. Here's a WIP patch which does that by eliminating >> intermediate ConvertRowtypeExprs during ExecInitExprRec(). > > Why is this done appropriately at ExecInitExpr() time, rather than at > plan time? Seems like eval_const_expressions() would be a bit more > appropriate (being badly named aside...)? That seems to be a better idea. Here's patch. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Вложения
On Tue, Apr 3, 2018 at 10:48 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
>>
>> Why is this done appropriately at ExecInitExpr() time, rather than at
>> plan time? Seems like eval_const_expressions() would be a bit more
>> appropriate (being badly named aside...)?
>
> That seems to be a better idea. Here's patch.
>
Previous patch didn't try to fold the ConvertRowtypeExpr::arg into a Const.
postgres=# create table t1 (a int, b int, c int) partition by range(a);
postgres=# create table t1p1 partition of t1 for values from (0) to
(100) partition by range(b);
postgres=# create table t1p1p1 partition of t1p1 for values from (0) to (50);
postgres=# explain verbose select (1, 2, 3)::t1p1p1::t1p1::t1; --
notice Rowexpression here.
QUERY PLAN
-------------------------------------------
Result (cost=0.00..0.01 rows=1 width=32)
Output: (ROW(1, 2, 3)::t1p1p1)::t1
(2 rows)
Here's patch fixing that. With this patch
postgres=# explain verbose select (1, 2, 3)::t1p1p1::t1p1::t1;
QUERY PLAN
-------------------------------------------
Result (cost=0.00..0.01 rows=1 width=32)
Output: '(1,2,3)'::t1
(2 rows)
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Вложения
2018-04-06 8:21 GMT+02:00 Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>:
+1
On Tue, Apr 3, 2018 at 10:48 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
>>
>> Why is this done appropriately at ExecInitExpr() time, rather than at
>> plan time? Seems like eval_const_expressions() would be a bit more
>> appropriate (being badly named aside...)?
>
> That seems to be a better idea. Here's patch.
>
Previous patch didn't try to fold the ConvertRowtypeExpr::arg into a Const.
postgres=# create table t1 (a int, b int, c int) partition by range(a);
postgres=# create table t1p1 partition of t1 for values from (0) to
(100) partition by range(b);
postgres=# create table t1p1p1 partition of t1p1 for values from (0) to (50);
postgres=# explain verbose select (1, 2, 3)::t1p1p1::t1p1::t1; --
notice Rowexpression here.
QUERY PLAN
-------------------------------------------
Result (cost=0.00..0.01 rows=1 width=32)
Output: (ROW(1, 2, 3)::t1p1p1)::t1
(2 rows)
Here's patch fixing that. With this patch
postgres=# explain verbose select (1, 2, 3)::t1p1p1::t1p1::t1;
QUERY PLAN
-------------------------------------------
Result (cost=0.00..0.01 rows=1 width=32)
Output: '(1,2,3)'::t1
(2 rows)
+1
Pavel
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
At Fri, 6 Apr 2018 08:37:57 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in <CAFj8pRAR9dL6Hw8EMb=QLHn-_WvafZNV9R40A4fZHr+qd7KXPg@mail.gmail.com> > 2018-04-06 8:21 GMT+02:00 Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>: > > > On Tue, Apr 3, 2018 at 10:48 AM, Ashutosh Bapat > > <ashutosh.bapat@enterprisedb.com> wrote: > > >> > > >> Why is this done appropriately at ExecInitExpr() time, rather than at > > >> plan time? Seems like eval_const_expressions() would be a bit more > > >> appropriate (being badly named aside...)? > > > > > > That seems to be a better idea. Here's patch. > > > > > > > Previous patch didn't try to fold the ConvertRowtypeExpr::arg into a Const. > > > > postgres=# create table t1 (a int, b int, c int) partition by range(a); > > postgres=# create table t1p1 partition of t1 for values from (0) to > > (100) partition by range(b); > > postgres=# create table t1p1p1 partition of t1p1 for values from (0) to > > (50); > > postgres=# explain verbose select (1, 2, 3)::t1p1p1::t1p1::t1; -- > > notice Rowexpression here. > > QUERY PLAN > > ------------------------------------------- > > Result (cost=0.00..0.01 rows=1 width=32) > > Output: (ROW(1, 2, 3)::t1p1p1)::t1 > > (2 rows) > > > > Here's patch fixing that. With this patch > > postgres=# explain verbose select (1, 2, 3)::t1p1p1::t1p1::t1; > > QUERY PLAN > > ------------------------------------------- > > Result (cost=0.00..0.01 rows=1 width=32) > > Output: '(1,2,3)'::t1 > > (2 rows) > > > > > +1 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. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
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? -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
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?
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
At Mon, 9 Apr 2018 15:53:04 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in <CAFjFpReDrtM8YVmTBgHHK3p8P9wEpKPO=YurkbqukM5c1oa0cQ@mail.gmail.com> > 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. I apologize for the unreadable statement.. > What output do you see without the patch? I got the following on the master. > Output: ((ROW(i.i, (i.i * 2), (i.i * 3))::t1p1p1)::t1p1)::t1 And I expect the following with this patch. > Output: ROW(i.i, (i.i * 2), (i.i * 3))::t1 And what the current patch generates looks imcomplete to me. > >> Output: (ROW(i.i, (i.i * 2), (i.i * 3))::t1p1p1)::t1 I try to reword the unreadable thing.. We get the similar composition of casts on scalar values. > =# explain verbose select 1::text::int::float; .. > Output: '1'::double precision But we don't get the same on non-constant. > =# explain verbose select x::text::int::float from generate_series(0, 10) x; ... > Output: (((x)::text)::integer)::double precision This seems reasonable since we cannot assume that "::double precision" and "::text::integer::double precision" are equivelant on arbitrary (or undecided, anyway I'm not sure it is true) input. But ConvertRowtypeExpr seems to be composable (or mergeable) for arbitrary input. Otherwise composition (or merging) of ConvertRowtypeExpr should not be performed at all. # I wish this makes sense.. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
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? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
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. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
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
On Mon, Apr 9, 2018 at 5:34 PM, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > > 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. Yes. I have modelled it after RelableType case few lines above in the same function. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
> On Mon, 9 Apr 2018 at 14:16, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > > On Mon, Apr 9, 2018 at 5:34 PM, Kyotaro HORIGUCHI > <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > > > > 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. > > Yes. I have modelled it after RelableType case few lines above in the > same function. This patch went through the last tree commit fests without any noticeable activity, but cfbot says it still applies and doesn't break any tests. The patch itself is rather small, and I could reproduce ~20% of performance improvements while running the same scripts under pgbench (although not in all cases), but probably we need to find someone to take over it. Does anyone wants to do so, maybe Kyotaro?
>>>>> "Dmitry" == Dmitry Dolgov <9erthalion6@gmail.com> writes: Dmitry> This patch went through the last tree commit fests without any Dmitry> noticeable activity, but cfbot says it still applies and Dmitry> doesn't break any tests. The patch itself is rather small, and Dmitry> I could reproduce ~20% of performance improvements while Dmitry> running the same scripts under pgbench (although not in all Dmitry> cases), but probably we need to find someone to take over it. Dmitry> Does anyone wants to do so, maybe Kyotaro? I'll deal with it. -- Andrew (irc:RhodiumToad)
> On Sun, 4 Nov 2018 at 15:48, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > > >>>>> "Dmitry" == Dmitry Dolgov <9erthalion6@gmail.com> writes: > > Dmitry> This patch went through the last tree commit fests without any > Dmitry> noticeable activity, but cfbot says it still applies and > Dmitry> doesn't break any tests. The patch itself is rather small, and > Dmitry> I could reproduce ~20% of performance improvements while > Dmitry> running the same scripts under pgbench (although not in all > Dmitry> cases), but probably we need to find someone to take over it. > Dmitry> Does anyone wants to do so, maybe Kyotaro? > > I'll deal with it. Thanks!
Sorry for the absense. At Sun, 4 Nov 2018 16:26:12 +0100, Dmitry Dolgov <9erthalion6@gmail.com> wrote in <CA+q6zcWMCNNmMQ-csuDf0Pqr1_ESat5-Vcu5uognfS3EaC4Apg@mail.gmail.com> > > On Sun, 4 Nov 2018 at 15:48, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > > > > >>>>> "Dmitry" == Dmitry Dolgov <9erthalion6@gmail.com> writes: > > > > Dmitry> This patch went through the last tree commit fests without any > > Dmitry> noticeable activity, but cfbot says it still applies and > > Dmitry> doesn't break any tests. The patch itself is rather small, and > > Dmitry> I could reproduce ~20% of performance improvements while > > Dmitry> running the same scripts under pgbench (although not in all > > Dmitry> cases), but probably we need to find someone to take over it. > > Dmitry> Does anyone wants to do so, maybe Kyotaro? > > > > I'll deal with it. > > Thanks! My last comment was the while() loop does nothing. Ashutosh said that it is on the model of RelabelType. I examined the code for T_RelabelType again and it is intending that the ece_mutator can convert something (specifically only CollateExpr) to RelableType, then reduce the nested Relabels. So the order is not wrong in this perspective. So I don't object to the current patch, but it needs test like attached. regards. -- Kyotaro Horiguchi NTT Open Source Software Center From 45ef5b753069eb89ab5139828884fbdbf0958778 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp> Date: Tue, 6 Nov 2018 17:12:17 +0900 Subject: [PATCH 2/2] Test code for Optimize nested ConvertRowtypExprs --- src/test/regress/expected/inherit.out | 18 ++++++++++++++++++ src/test/regress/sql/inherit.sql | 5 +++++ 2 files changed, 23 insertions(+) diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 4f29d9f891..1474ed8190 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -764,6 +764,8 @@ NOTICE: drop cascades to table c1 -- tables. See the pgsql-hackers thread beginning Dec. 4/04 create table base (i integer); create table derived () inherits (base); +create table more_derived (like derived, b int) inherits (derived); +NOTICE: merging column "i" with inherited definition insert into derived (i) values (0); select derived::base from derived; derived @@ -777,6 +779,22 @@ select NULL::derived::base; (1 row) +-- remove redundant conversions. +explain (verbose on, costs off) select row(i, b)::more_derived::derived::base from more_derived; + QUERY PLAN +------------------------------------------- + Seq Scan on public.more_derived + Output: (ROW(i, b)::more_derived)::base +(2 rows) + +explain (verbose on, costs off) select (1, 2)::more_derived::derived::base; + QUERY PLAN +----------------------- + Result + Output: '(1)'::base +(2 rows) + +drop table more_derived; drop table derived; drop table base; create table p1(ff1 int); diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index a6e541d4da..8308330fed 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -237,9 +237,14 @@ drop table p1 cascade; -- tables. See the pgsql-hackers thread beginning Dec. 4/04 create table base (i integer); create table derived () inherits (base); +create table more_derived (like derived, b int) inherits (derived); insert into derived (i) values (0); select derived::base from derived; select NULL::derived::base; +-- remove redundant conversions. +explain (verbose on, costs off) select row(i, b)::more_derived::derived::base from more_derived; +explain (verbose on, costs off) select (1, 2)::more_derived::derived::base; +drop table more_derived; drop table derived; drop table base; -- 2.16.3
>>>>> "Kyotaro" == Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes: Kyotaro> My last comment was the while() loop does nothing. Ashutosh Kyotaro> said that it is on the model of RelabelType. Kyotaro> I examined the code for T_RelabelType again and it is Kyotaro> intending that the ece_mutator can convert something Kyotaro> (specifically only CollateExpr) to RelableType, then reduce Kyotaro> the nested Relabels. So the order is not wrong in this Kyotaro> perspective. I'm eliminating the while loop in favour of an if, because I also think it a good idea to ensure that the resulting convertformat is explicit if any of the component conversions is explicit (rather than relying on the top of the stack to be the most explicit one). Kyotaro> So I don't object to the current patch, but it needs test like Kyotaro> attached. Thanks. I'm going to pull all this together and commit it shortly. -- Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
Andrew> I'm going to pull all this together and commit it shortly.
Here's the patch with my edits (more comments and the while/if change).
I'll commit this in due course unless I hear otherwise.
--
Andrew (irc:RhodiumToad)
From 9cc81cea6de41140fe361bff375190bfdd188ae9 Mon Sep 17 00:00:00 2001
From: Andrew Gierth <rhodiumtoad@postgresql.org>
Date: Tue, 6 Nov 2018 14:19:40 +0000
Subject: [PATCH] Optimize nested ConvertRowtypeExpr nodes.
A ConvertRowtypeExpr is used to translate a whole-row reference of a
child to that of a parent. The planner produces nested
ConvertRowtypeExpr while translating whole-row reference of a leaf
partition in a multi-level partition hierarchy. Executor then
translates the whole-row reference from the leaf partition into all
the intermediate parent's whole-row references before arriving at the
final whole-row reference. It could instead translate the whole-row
reference from the leaf partition directly to the top-most parent's
whole-row reference skipping any intermediate translations.
Ashutosh Bapat, with tests by Kyotaro Horiguchi and some
editorialization by me. Reviewed by Andres Freund, Pavel Stehule,
Kyotaro Horiguchi, Dmitry Dolgov.
---
src/backend/optimizer/util/clauses.c | 46 +++++++++++++++++++++++++++++++++++
src/test/regress/expected/inherit.out | 18 ++++++++++++++
src/test/regress/sql/inherit.sql | 5 ++++
3 files changed, 69 insertions(+)
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 21bf5dea9c..d13c3ac895 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -3716,6 +3716,52 @@ eval_const_expressions_mutator(Node *node,
context);
}
break;
+ case T_ConvertRowtypeExpr:
+ {
+ ConvertRowtypeExpr *cre = castNode(ConvertRowtypeExpr, node);
+ Node *arg;
+ ConvertRowtypeExpr *newcre;
+
+ arg = eval_const_expressions_mutator((Node *) cre->arg,
+ context);
+
+ newcre = makeNode(ConvertRowtypeExpr);
+ newcre->resulttype = cre->resulttype;
+ newcre->convertformat = cre->convertformat;
+ newcre->location = cre->location;
+
+ /*
+ * In case of a nested ConvertRowtypeExpr, we can convert the
+ * leaf row directly to the topmost row format without any
+ * intermediate conversions. (This works because
+ * ConvertRowtypeExpr is used only for child->parent
+ * conversion in inheritance trees, which works by exact match
+ * of column name, and a column absent in an intermediate
+ * result can't be present in the final result.)
+ *
+ * No need to check more than one level deep, because the
+ * above recursion will have flattened anything else.
+ */
+ if (arg != NULL && IsA(arg, ConvertRowtypeExpr))
+ {
+ ConvertRowtypeExpr *argcre = castNode(ConvertRowtypeExpr, arg);
+
+ arg = (Node *) argcre->arg;
+
+ /*
+ * Make sure an outer implicit conversion can't hide an
+ * inner explicit one.
+ */
+ if (newcre->convertformat == COERCE_IMPLICIT_CAST)
+ newcre->convertformat = argcre->convertformat;
+ }
+
+ newcre->arg = (Expr *) arg;
+
+ if (arg != NULL && IsA(arg, Const))
+ return ece_evaluate_expr((Node *) newcre);
+ return (Node *) newcre;
+ }
default:
break;
}
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index d768e5df2c..1e00c849f3 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -764,6 +764,8 @@ NOTICE: drop cascades to table c1
-- tables. See the pgsql-hackers thread beginning Dec. 4/04
create table base (i integer);
create table derived () inherits (base);
+create table more_derived (like derived, b int) inherits (derived);
+NOTICE: merging column "i" with inherited definition
insert into derived (i) values (0);
select derived::base from derived;
derived
@@ -777,6 +779,22 @@ select NULL::derived::base;
(1 row)
+-- remove redundant conversions.
+explain (verbose on, costs off) select row(i, b)::more_derived::derived::base from more_derived;
+ QUERY PLAN
+-------------------------------------------
+ Seq Scan on public.more_derived
+ Output: (ROW(i, b)::more_derived)::base
+(2 rows)
+
+explain (verbose on, costs off) select (1, 2)::more_derived::derived::base;
+ QUERY PLAN
+-----------------------
+ Result
+ Output: '(1)'::base
+(2 rows)
+
+drop table more_derived;
drop table derived;
drop table base;
create table p1(ff1 int);
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index e8b6448f3c..afc72f47bc 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -237,9 +237,14 @@ drop table p1 cascade;
-- tables. See the pgsql-hackers thread beginning Dec. 4/04
create table base (i integer);
create table derived () inherits (base);
+create table more_derived (like derived, b int) inherits (derived);
insert into derived (i) values (0);
select derived::base from derived;
select NULL::derived::base;
+-- remove redundant conversions.
+explain (verbose on, costs off) select row(i, b)::more_derived::derived::base from more_derived;
+explain (verbose on, costs off) select (1, 2)::more_derived::derived::base;
+drop table more_derived;
drop table derived;
drop table base;
--
2.11.1
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> Here's the patch with my edits (more comments and the while/if change).
A couple minor thoughts:
* I dislike using castNode() in places where the code has just explicitly
verified the node type, which is true in both places where you used it
here. The assertion is just bulking the code up to no purpose, and it
creates an unnecessary discrepancy between older and newer code.
* As you have it here, a construct such as
ConvertRowtypeExpr(ConvertRowtypeExpr(ConvertRowtypeExpr(Const)))
will laboriously perform each of the intermediate steps, which seems
like exactly the case we're trying to prevent at runtime. I wonder
whether it is worth stripping off ConvertRowtypeExpr's before the
recursive eval_const_expressions_mutator call to prevent that.
You'd still want the check after the call, to handle situations where
something more complex got simplified to a ConvertRowtypeExpr; and this
would also complicate getting the convertformat right. So perhaps it's
not worth the trouble, but I thought I'd mention it.
* I find the hardwired logic about how to merge distinct convertformat
values a bit troublesome. Maybe use Min() instead? Although there
is not currently any expectation about the ordering of that enum ...
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> * I dislike using castNode() in places where the code has just Tom> explicitly verified the node type, which is true in both places Tom> where you used it here. The assertion is just bulking the code up Tom> to no purpose, and it creates an unnecessary discrepancy between Tom> older and newer code. hmm... fair point, I'll think about it Tom> * As you have it here, a construct such as Tom> ConvertRowtypeExpr(ConvertRowtypeExpr(ConvertRowtypeExpr(Const))) Tom> will laboriously perform each of the intermediate steps, which Tom> seems like exactly the case we're trying to prevent at runtime. I Tom> wonder whether it is worth stripping off ConvertRowtypeExpr's Tom> before the recursive eval_const_expressions_mutator call to Tom> prevent that. You'd still want the check after the call, to handle Tom> situations where something more complex got simplified to a Tom> ConvertRowtypeExpr; and this would also complicate getting the Tom> convertformat right. So perhaps it's not worth the trouble, but I Tom> thought I'd mention it. I think it's not worth the trouble. Tom> * I find the hardwired logic about how to merge distinct Tom> convertformat values a bit troublesome. Maybe use Min() instead? Tom> Although there is not currently any expectation about the ordering Tom> of that enum ... I considered using Min() but decided against it specifically _because_ there was no suggestion either in the enum definition, or in any other use of CoercionForm anywhere, that the order of values was intended to be significant. Since there's only one value for "implicit", it seemed better to work from that. -- Andrew (irc:RhodiumToad)