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
Attachments:
pg_nest_cre.patchtext/x-patch; charset=US-ASCII; name=pg_nest_cre.patchDownload
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index db5fcaf..ec896a4 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1364,6 +1364,37 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_ConvertRowtypeExpr:
{
ConvertRowtypeExpr *convert = (ConvertRowtypeExpr *) node;
+ ConvertRowtypeExpr *tmp_cre = convert;
+ bool nested_cre = false;
+
+ /*
+ * If this is a nested ConvertRowtypeExpr resulting from a
+ * multi-level partition/inheritance hierarchy, its leaf node
+ * will be a whole-row expression. We can convert the leaf
+ * whole-row directly into the topmost parent, without
+ * converting it into the intermediate parent row types.
+ */
+ while (IsA(tmp_cre->arg, ConvertRowtypeExpr))
+ {
+ tmp_cre = castNode(ConvertRowtypeExpr, tmp_cre->arg);
+ nested_cre = true;
+ }
+
+ if (nested_cre && IsA(tmp_cre->arg, Var) &&
+ castNode(Var, tmp_cre->arg)->varattno == 0)
+ {
+ ConvertRowtypeExpr *new_convert;
+
+ /*
+ * XXX: Instead of modifying the expression directly, we
+ * save a modified copy in the execution tree. May be it's
+ * safe to modify the expression directly, not sure.
+ */
+ new_convert = makeNode(ConvertRowtypeExpr);
+ memcpy(new_convert, convert, sizeof(ConvertRowtypeExpr));
+ new_convert->arg = tmp_cre->arg;
+ convert = new_convert;
+ }
/* evaluate argument into step's result area */
ExecInitExprRec(convert->arg, state, resv, resnull);
nest_cre.out.patchtext/x-patch; charset=US-ASCII; name=nest_cre.out.patchDownload
\set num_rows 1000000
drop table t1;
drop table p cascade;
create table t1 (a int, b varchar, c timestamp) partition by range(a);
create table t1p1 (b varchar, c timestamp, a int) partition by range(a);
alter table t1 attach partition t1p1 for values from (0) to (100);
create table t1p1p1(c timestamp, a int, b varchar);
alter table t1p1 attach partition t1p1p1 for values from (0) to (50);
insert into t1 select abs(random()) * 49, i, now() from generate_series(1, :num_rows) i;
explain analyze select t1 from t1;
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------
Append (cost=0.00..13565.97 rows=719697 width=32) (actual time=0.079..683.430 rows=1000000 loops=1)
-> Seq Scan on t1p1p1 (cost=0.00..13565.97 rows=719697 width=32) (actual time=0.077..616.439 rows=1000000 loops=1)
Planning time: 0.193 ms
Execution time: 717.929 ms
(4 rows)
explain analyze select t1 from t1;
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------
Append (cost=0.00..13565.97 rows=719697 width=32) (actual time=0.017..607.063 rows=1000000 loops=1)
-> Seq Scan on t1p1p1 (cost=0.00..13565.97 rows=719697 width=32) (actual time=0.016..541.619 rows=1000000 loops=1)
Planning time: 0.115 ms
Execution time: 640.628 ms
(4 rows)
explain analyze select t1 from t1;
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------
Append (cost=0.00..13565.97 rows=719697 width=32) (actual time=0.016..605.972 rows=1000000 loops=1)
-> Seq Scan on t1p1p1 (cost=0.00..13565.97 rows=719697 width=32) (actual time=0.016..541.256 rows=1000000 loops=1)
Planning time: 0.109 ms
Execution time: 639.426 ms
(4 rows)
explain analyze select t1 from t1;
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------
Append (cost=0.00..13565.97 rows=719697 width=32) (actual time=0.017..613.866 rows=1000000 loops=1)
-> Seq Scan on t1p1p1 (cost=0.00..13565.97 rows=719697 width=32) (actual time=0.017..548.002 rows=1000000 loops=1)
Planning time: 0.112 ms
Execution time: 647.820 ms
(4 rows)
explain analyze select t1 from t1;
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------
Append (cost=0.00..13565.97 rows=719697 width=32) (actual time=0.016..612.391 rows=1000000 loops=1)
-> Seq Scan on t1p1p1 (cost=0.00..13565.97 rows=719697 width=32) (actual time=0.016..546.605 rows=1000000 loops=1)
Planning time: 0.111 ms
Execution time: 646.292 ms
(4 rows)
create table p (a int, b varchar, c timestamp);
create table p_c (b varchar, c timestamp, a int, d int);
alter table p_c inherit p;
create table p_c_c (c timestamp, a int, b varchar, d int, e int);
alter table p_c_c inherit p_c;
insert into p_c_c select c, a, b, a, a from t1;
explain analyze select p from p;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
Append (cost=0.00..14851.04 rows=749906 width=32) (actual time=0.044..737.725 rows=1000000 loops=1)
-> Seq Scan on p (cost=0.00..0.00 rows=1 width=68) (actual time=0.002..0.002 rows=0 loops=1)
-> Seq Scan on p_c (cost=0.00..0.00 rows=1 width=32) (actual time=0.000..0.000 rows=0 loops=1)
-> Seq Scan on p_c_c (cost=0.00..14851.04 rows=749904 width=32) (actual time=0.042..672.052 rows=1000000 loops=1)
Planning time: 0.194 ms
Execution time: 772.241 ms
(6 rows)
explain analyze select p from p;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
Append (cost=0.00..14851.04 rows=749906 width=32) (actual time=0.018..677.591 rows=1000000 loops=1)
-> Seq Scan on p (cost=0.00..0.00 rows=1 width=68) (actual time=0.001..0.001 rows=0 loops=1)
-> Seq Scan on p_c (cost=0.00..0.00 rows=1 width=32) (actual time=0.001..0.001 rows=0 loops=1)
-> Seq Scan on p_c_c (cost=0.00..14851.04 rows=749904 width=32) (actual time=0.016..611.849 rows=1000000 loops=1)
Planning time: 0.098 ms
Execution time: 711.619 ms
(6 rows)
explain analyze select p from p;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
Append (cost=0.00..14851.04 rows=749906 width=32) (actual time=0.018..684.336 rows=1000000 loops=1)
-> Seq Scan on p (cost=0.00..0.00 rows=1 width=68) (actual time=0.001..0.001 rows=0 loops=1)
-> Seq Scan on p_c (cost=0.00..0.00 rows=1 width=32) (actual time=0.001..0.001 rows=0 loops=1)
-> Seq Scan on p_c_c (cost=0.00..14851.04 rows=749904 width=32) (actual time=0.015..618.316 rows=1000000 loops=1)
Planning time: 0.104 ms
Execution time: 718.329 ms
(6 rows)
explain analyze select p from p;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
Append (cost=0.00..14851.04 rows=749906 width=32) (actual time=0.018..677.647 rows=1000000 loops=1)
-> Seq Scan on p (cost=0.00..0.00 rows=1 width=68) (actual time=0.001..0.001 rows=0 loops=1)
-> Seq Scan on p_c (cost=0.00..0.00 rows=1 width=32) (actual time=0.000..0.000 rows=0 loops=1)
-> Seq Scan on p_c_c (cost=0.00..14851.04 rows=749904 width=32) (actual time=0.016..611.973 rows=1000000 loops=1)
Planning time: 0.098 ms
Execution time: 711.535 ms
(6 rows)
explain analyze select p from p;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
Append (cost=0.00..14851.04 rows=749906 width=32) (actual time=0.018..675.063 rows=1000000 loops=1)
-> Seq Scan on p (cost=0.00..0.00 rows=1 width=68) (actual time=0.001..0.001 rows=0 loops=1)
-> Seq Scan on p_c (cost=0.00..0.00 rows=1 width=32) (actual time=0.001..0.001 rows=0 loops=1)
-> Seq Scan on p_c_c (cost=0.00..14851.04 rows=749904 width=32) (actual time=0.016..609.450 rows=1000000 loops=1)
Planning time: 0.098 ms
Execution time: 708.892 ms
(6 rows)
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
Attachments:
0001-Optimize-nested-ConvertRowtypeExprs.patchtext/x-patch; charset=US-ASCII; name=0001-Optimize-nested-ConvertRowtypeExprs.patchDownload
From b99ae826931516a7c6ad981435e8ac9ef754f15e Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Date: Tue, 3 Apr 2018 09:00:19 +0530
Subject: [PATCH] Optimize nested ConvertRowtypeExprs
A ConvertRowtypeExprs 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, reviewed by Andres Freund.
---
src/backend/optimizer/util/clauses.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index ed6b680..3af5033 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -3657,6 +3657,31 @@ eval_const_expressions_mutator(Node *node,
context);
}
break;
+ case T_ConvertRowtypeExpr:
+ {
+ ConvertRowtypeExpr *cre = castNode(ConvertRowtypeExpr, node);
+ Expr *arg = cre->arg;
+ ConvertRowtypeExpr *newcre;
+
+ /*
+ * In case of a nested ConvertRowtypeExpr, we can convert the
+ * leaf row directly to the topmost row format without any
+ * intermediate conversions.
+ */
+ Assert(arg != NULL);
+ while (IsA(arg, ConvertRowtypeExpr))
+ arg = castNode(ConvertRowtypeExpr, arg)->arg;
+
+ newcre = makeNode(ConvertRowtypeExpr);
+ newcre->arg = arg;
+ newcre->resulttype = cre->resulttype;
+ newcre->convertformat = cre->convertformat;
+ newcre->location = newcre->location;
+
+ if (IsA(arg, Const))
+ return ece_evaluate_expr((Node *) newcre);
+ return (Node *) newcre;
+ }
default:
break;
}
--
1.7.9.5
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
Attachments:
0001-Optimize-nested-ConvertRowtypeExprs_v2.patchtext/x-patch; charset=US-ASCII; name=0001-Optimize-nested-ConvertRowtypeExprs_v2.patchDownload
From aade26da4b45b96c82ac44ad93c7d8a9efc03dd2 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Date: Tue, 3 Apr 2018 09:00:19 +0530
Subject: [PATCH] Optimize nested ConvertRowtypeExprs
A ConvertRowtypeExprs 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, reviewed by Andres Freund.
---
src/backend/optimizer/util/clauses.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index ed6b680..d11cb72 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -3657,6 +3657,33 @@ 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);
+
+ /*
+ * 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;
+
+ newcre = makeNode(ConvertRowtypeExpr);
+ newcre->arg = (Expr *) arg;
+ newcre->resulttype = cre->resulttype;
+ newcre->convertformat = cre->convertformat;
+ newcre->location = cre->location;
+
+ if (arg != NULL && IsA(arg, Const))
+ return ece_evaluate_expr((Node *) newcre);
+ return (Node *) newcre;
+ }
default:
break;
}
--
1.7.9.5
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
Pavel
Show quoted text
--
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
Attachments:
0002-Test-code-for-Optimize-nested-ConvertRowtypExprs.patchtext/x-patch; charset=us-asciiDownload
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)
Attachments:
0001-Optimize-nested-ConvertRowtypeExpr-nodes.patchtext/x-patchDownload
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)