"SELECT ... FROM DUAL" is not quite as silly as it appears
We've long made fun of Oracle(TM) for the fact that if you just want
to evaluate some expressions, you have to write "select ... from dual"
rather than just "select ...". But I've realized recently that there's
a bit of method in that madness after all. Specifically, having to
cope with FromExprs that contain no base relation is fairly problematic
in the planner. prepjointree.c is basically unable to cope with
flattening a subquery that looks that way, although we've inserted a
lot of overly-baroque logic to handle some subsets of the case (cf
is_simple_subquery(), around line 1500). If memory serves, there are
other places that are complicated by the case.
Suppose that, either in the rewriter or early in the planner, we were
to replace such cases with nonempty FromExprs, by adding a dummy RTE
representing a table with no columns and one row. This would in turn
give rise to an ordinary Path that converts to a Result plan, so that
the case is handled without any special contortions later. Then there
is no case where we don't have a nonempty relids set identifying a
subquery, so that all that special-case hackery in prepjointree.c
goes away, and we can simplify whatever else is having a hard time
with it.
I'm not planning to do anything about this soon (ie, not before v12),
but I thought I'd get the ideas down on electrons before they vanish.
regards, tom lane
On Thu, Mar 15, 2018 at 11:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We've long made fun of Oracle(TM) for the fact that if you just want
to evaluate some expressions, you have to write "select ... from dual"
rather than just "select ...". But I've realized recently that there's
a bit of method in that madness after all. Specifically, having to
cope with FromExprs that contain no base relation is fairly problematic
in the planner. prepjointree.c is basically unable to cope with
flattening a subquery that looks that way, although we've inserted a
lot of overly-baroque logic to handle some subsets of the case (cf
is_simple_subquery(), around line 1500). If memory serves, there are
other places that are complicated by the case.Suppose that, either in the rewriter or early in the planner, we were
to replace such cases with nonempty FromExprs, by adding a dummy RTE
representing a table with no columns and one row. This would in turn
give rise to an ordinary Path that converts to a Result plan, so that
the case is handled without any special contortions later. Then there
is no case where we don't have a nonempty relids set identifying a
subquery, so that all that special-case hackery in prepjointree.c
goes away, and we can simplify whatever else is having a hard time
with it.
Good idea.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Mar 15, 2018 at 8:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We've long made fun of Oracle(TM) for the fact that if you just want
to evaluate some expressions, you have to write "select ... from dual"
rather than just "select ...". But I've realized recently that there's
a bit of method in that madness after all. Specifically, having to
cope with FromExprs that contain no base relation is fairly problematic
in the planner. prepjointree.c is basically unable to cope with
flattening a subquery that looks that way, although we've inserted a
lot of overly-baroque logic to handle some subsets of the case (cf
is_simple_subquery(), around line 1500). If memory serves, there are
other places that are complicated by the case.Suppose that, either in the rewriter or early in the planner, we were
to replace such cases with nonempty FromExprs, by adding a dummy RTE
representing a table with no columns and one row. This would in turn
give rise to an ordinary Path that converts to a Result plan, so that
the case is handled without any special contortions later. Then there
is no case where we don't have a nonempty relids set identifying a
subquery, so that all that special-case hackery in prepjointree.c
goes away, and we can simplify whatever else is having a hard time
with it.
The idea looks neat.
Since table in the dummy FROM clause returns one row without any
column, I guess, there will be at least one row in the output. I am
curious how would we handle cases which do not return any row
like
create function set_ret_func() returns setof record as $$select * from
pg_class where oid = 0;$$ language sql;
select set_ret_func();
set_ret_func
--------------
(0 rows)
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
On Thu, Mar 15, 2018 at 8:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Suppose that, either in the rewriter or early in the planner, we were
to replace such cases with nonempty FromExprs, by adding a dummy RTE
representing a table with no columns and one row. This would in turn
give rise to an ordinary Path that converts to a Result plan, so that
the case is handled without any special contortions later.
Since table in the dummy FROM clause returns one row without any
column, I guess, there will be at least one row in the output. I am
curious how would we handle cases which do not return any row
like
create function set_ret_func() returns setof record as $$select * from
pg_class where oid = 0;$$ language sql;
select set_ret_func();
set_ret_func
--------------
(0 rows)
It'd be the same as now, so far as the executor is concerned:
regression=# explain select set_ret_func();
QUERY PLAN
--------------------------------------------------
ProjectSet (cost=0.00..5.27 rows=1000 width=32)
-> Result (cost=0.00..0.01 rows=1 width=0)
(2 rows)
The difference is that, within the planner, the ResultPath would be
associated with a "real" base relation instead of needing its very
own code path in query_planner().
regards, tom lane
Robert Haas <robertmhaas@gmail.com> writes:
On Thu, Mar 15, 2018 at 11:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We've long made fun of Oracle(TM) for the fact that if you just want
to evaluate some expressions, you have to write "select ... from dual"
rather than just "select ...". But I've realized recently that there's
a bit of method in that madness after all. Specifically, having to
cope with FromExprs that contain no base relation is fairly problematic
in the planner. prepjointree.c is basically unable to cope with
flattening a subquery that looks that way, although we've inserted a
lot of overly-baroque logic to handle some subsets of the case (cf
is_simple_subquery(), around line 1500). If memory serves, there are
other places that are complicated by the case.Suppose that, either in the rewriter or early in the planner, we were
to replace such cases with nonempty FromExprs, by adding a dummy RTE
representing a table with no columns and one row. This would in turn
give rise to an ordinary Path that converts to a Result plan, so that
the case is handled without any special contortions later. Then there
is no case where we don't have a nonempty relids set identifying a
subquery, so that all that special-case hackery in prepjointree.c
goes away, and we can simplify whatever else is having a hard time
with it.
Good idea.
Here's a proposed patch along those lines. Some notes for review:
* The new RTE kind is "RTE_RESULT" after the kind of Plan node it will
produce. I'm not entirely in love with that name, but couldn't think
of a better idea.
* I renamed the existing ResultPath node type to GroupResultPath to
clarify that it's not used to scan RTE_RESULT RTEs, but just for
degenerate grouping cases. RTE_RESULT RTEs (usually) give rise to plain
Path nodes with pathtype T_Result. It doesn't work very well to try to
unify those two cases, even though they give rise to identical Plans,
because there are different rules about where the associated quals live
during query_planner.
* The interesting changes are in prepjointree.c; almost all the rest
of the patch is boilerplate to support RTE_RESULT RTEs in mostly the
same way that other special RTE-scan plan types are handled in the
planner. In prepjointree.c, I ended up getting rid of the original
decision to try to delete removable RTEs during pull_up_subqueries,
and instead made it happen in a separate traversal of the join tree.
That's a lot less complicated, and it has better results because we
can optimize more cases once we've seen the results of expression
preprocessing and outer-join strength reduction.
* I tried to get rid of the empty-jointree special case in query_planner
altogether. While the patch works fine that way, it makes for a
measurable slowdown in planning trivial queries --- I saw close to 10%
degradation in TPS rate for a pgbench test case that was just
$ cat trivialselect.sql
select 2+2;
$ pgbench -n -T 10 -f trivialselect.sql
So I ended up putting back the special case, but it's much less of a
cheat than it was before; the RelOptInfo it hands back is basically the
same as the normal path would produce. For me, the patch as given is
within the noise level of being the same speed as HEAD on this case.
* There's a hack in nodeResult.c to prevent the executor from crashing
on a whole-row Var for an RTE_RESULT RTE, which is something that the
planner will create in SELECT FOR UPDATE cases, because it thinks it
needs to provide a ROW_MARK_COPY image of the RTE's output. We might
be able to get rid of that if we could teach the planner that it need
not bother rowmarking RESULT RTEs, but that seemed like it would be
really messy. (At the point where the decision is made, we don't know
whether a subquery might end up as just a RESULT, or indeed vanish
entirely.) Since I couldn't measure any reproducible penalty from
having the extra setup cost for a Result plan, I left it like this.
* There are several existing test cases in join.sql whose plans change
for the better with this patch, so I didn't really think we needed any
additional test cases to show that it's working.
* There are a couple of cosmetic changes in EXPLAIN output as a result
of ruleutils.c seeing more RTEs in the plan's rtable than it did before,
causing it to decide to table-qualify Var names in more cases. We could
maybe spend more effort on ruleutils.c's heuristic for when to qualify,
but that seems like a separable problem, and anyway it's only cosmetic.
I'll add this to the next CF.
regards, tom lane
Attachments:
get-rid-of-empty-jointrees-1.patchtext/x-diff; charset=us-ascii; name=get-rid-of-empty-jointrees-1.patchDownload+1012-422
On Fri, Mar 16, 2018 at 4:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
We've long made fun of Oracle(TM) for the fact that if you just want
to evaluate some expressions, you have to write "select ... from dual"
rather than just "select ...". But I've realized recently that there's
a bit of method in that madness after all.
We can still make fun of that table. Apparently it had two rows, so
you could double rows by cross joining against it, but at some point
one of them went missing, leaving a strange name behind. Source:
https://en.wikipedia.org/wiki/DUAL_table#History
--
Thomas Munro
http://www.enterprisedb.com
I wrote:
* There's a hack in nodeResult.c to prevent the executor from crashing
on a whole-row Var for an RTE_RESULT RTE, which is something that the
planner will create in SELECT FOR UPDATE cases, because it thinks it
needs to provide a ROW_MARK_COPY image of the RTE's output. We might
be able to get rid of that if we could teach the planner that it need
not bother rowmarking RESULT RTEs, but that seemed like it would be
really messy. (At the point where the decision is made, we don't know
whether a subquery might end up as just a RESULT, or indeed vanish
entirely.) Since I couldn't measure any reproducible penalty from
having the extra setup cost for a Result plan, I left it like this.
Well, I'd barely sent this when I realized that there was a better way.
The nodeResult.c hack predates my decision to postpone cleaning up
RTE_RESULT RTEs till near the end of the preprocessing phase, and
given that code, it is easy to get rid of rowmarking RESULT RTEs ...
in fact, the code was doing it already, except in the edge case of
a SELECT with only a RESULT RTE. So here's a version that does not
touch nodeResult.c.
regards, tom lane
Attachments:
get-rid-of-empty-jointrees-2.patchtext/x-diff; charset=us-ascii; name=get-rid-of-empty-jointrees-2.patchDownload+1015-422
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Patch applies cleanly on master (b238527664ec6f6c9d00dba4cc2f3dab1c8b8b04), compiles, and passes both 'make check-world' and 'make installcheck-world'.
The patch includes changes to the expected output of a few tests, and adds new code comments and changes existing code comments, but I did not notice any new tests or new documentation to specifically test or explain the behavioral change this patch is intended to introduce. None of the code comments seem to adequately explain what an RTE_RESULT is and when it would be used. This information can be gleaned with some difficulty by reading every file containing RTE_RESULT, but that seems rather unfriendly.
As an example of where I could use a bit more documentation, see src/backend/rewrite/rewriteHandler.c circa line 447; I don't know why the switch statement lacks a case for RTE_RESULT. Why would RTE_VALUES contain bare expressions but RTE_RESULT would not? Does this mean that
INSERT INTO mytable VALUES ('foo', 'bar');
differs from
SELECT 'foo', 'bar';
in terms of whether 'foo' and 'bar' are bare expressions? Admittedly, I don't know this code very well, and this might be obvious to others.
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Sorry about the prior review; I neglected to select all the appropriate buttons, leading to an errant "tested, failed" in the review.
I was also looking at this patch, here are some things I noticed:
In remove_useless_results_recurse where it processes JOIN_SEMI there is
this comment:
* However, we can't simplify if there are PHVs to
evaluate at
* the RTE_RESULT ... but that's impossible isn't it?
Is that impossible because the RHS of semi join can't be used above it?
Let's write this down. There is similar code above for JOIN_LEFT and it
does have to check for PHVs, so a comment that clarifies the reasons for
the difference would help.
Also around there:
if ((varno = is_result_ref(root, j->rarg)) != 0 &&
I'd expect a function that starts with "is_" to return a bool, so this
catches the eye. Maybe varno = get_result_relid()?
Looking at the coverage report of regression tests, most of the new code
is covered except for the aforementioned simplification of JOIN_LEFT and
JOIN_RIGHT, but it's probably not worth adding a special test. I checked
these cases manually and they work OK.
I also repeated the benchmark with trivial select and can confirm that
there is no change in performance.
--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Oh, one more thing: I see a warning without --enable-cassert in
create_resultscan_plan, because rte is only used in an Assert.
src/backend/optimizer/plan/createplan.c:3457:17: warning: variable ‘rte’
set but not used [-Wunused-but-set-variable]
RangeTblEntry *rte;
--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Mark Dilger <hornschnorter@gmail.com> writes:
Patch applies cleanly on master (b238527664ec6f6c9d00dba4cc2f3dab1c8b8b04), compiles, and passes both 'make check-world' and 'make installcheck-world'.
Thanks for reviewing!
The patch includes changes to the expected output of a few tests, and adds new code comments and changes existing code comments, but I did not notice any new tests or new documentation to specifically test or explain the behavioral change this patch is intended to introduce. None of the code comments seem to adequately explain what an RTE_RESULT is and when it would be used. This information can be gleaned with some difficulty by reading every file containing RTE_RESULT, but that seems rather unfriendly.
Well, there's no user-facing documentation because it's not a user-facing
feature; it just exists to make some things simpler inside the planner.
I will admit that the comment for RTE_RESULT in parsenodes.h is a bit
vague; that's mostly because when I started on this patch, it wasn't clear
to me whether or not to have RTE_RESULT present from the beginning (i.e.
have the parser create one) or let the planner inject them. I ended up
doing the latter, so the attached update of the patch now says
@@ -950,7 +950,10 @@ typedef enum RTEKind
RTE_TABLEFUNC, /* TableFunc(.., column list) */
RTE_VALUES, /* VALUES (<exprlist>), (<exprlist>), ... */
RTE_CTE, /* common table expr (WITH list element) */
- RTE_NAMEDTUPLESTORE /* tuplestore, e.g. for AFTER triggers */
+ RTE_NAMEDTUPLESTORE, /* tuplestore, e.g. for AFTER triggers */
+ RTE_RESULT /* RTE represents an empty FROM clause; such
+ * RTEs are added by the planner, they're not
+ * present during parsing or rewriting */
} RTEKind;
typedef struct RangeTblEntry
I'm not sure if that's enough to address your concern or not. But none
of the other RTEKinds are documented much more than this, either ...
As an example of where I could use a bit more documentation, see
src/backend/rewrite/rewriteHandler.c circa line 447; I don't know why
the switch statement lacks a case for RTE_RESULT. Why would RTE_VALUES
contain bare expressions but RTE_RESULT would not?
Well, it just doesn't. The comments in struct RangeTblEntry are pretty
clear about which fields apply to which RTE kinds, and none of the ones
containing subexpressions are valid for RTE_RESULT. As to *why* it's
like that, it's because an empty FROM clause doesn't produce any columns,
by definition.
Does this mean that
INSERT INTO mytable VALUES ('foo', 'bar');
differs from
SELECT 'foo', 'bar';
in terms of whether 'foo' and 'bar' are bare expressions?
Well, it does if there are multiple rows of VALUES items; look at the
parser's transformInsertStmt, which does things differently for a
single-row VALUES than multiple rows. We only create a VALUES RTE
for the multi-rows case, for which "SELECT 'foo', 'bar'" doesn't work.
Attached is an updated patch that responds to your comments and
Alexander's, and also adds a test case for EvalPlanQual involving
a RTE_RESULT RTE, because I got worried about whether that really
worked.
regards, tom lane
Attachments:
get-rid-of-empty-jointrees-4.patchtext/x-diff; charset=us-ascii; name=get-rid-of-empty-jointrees-4.patchDownload+1072-428
Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru> writes:
I was also looking at this patch, here are some things I noticed:
Thanks for reviewing! I incorporated your suggestions in the v4
patch I just sent.
regards, tom lane
Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru> writes:
Oh, one more thing: I see a warning without --enable-cassert in
create_resultscan_plan, because rte is only used in an Assert.
src/backend/optimizer/plan/createplan.c:3457:17: warning: variable ‘rte’
set but not used [-Wunused-but-set-variable]
RangeTblEntry *rte;
Ooops, I had not seen this before sending v4 patch. Doesn't seem worth
posting a v5 for, but I'll be sure to fix it.
regards, tom lane
On 11/29/18 22:13, Tom Lane wrote:
Ooops, I had not seen this before sending v4 patch. Doesn't seem worth
posting a v5 for, but I'll be sure to fix it.
Thanks for updating, v4 looks good to me.
--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
I've just looked over the v4 patch. I agree that having an RTE for a
from-less SELECT seems like a good idea.
While reading the patch, I noted the following:
1. It's more efficient to use bms_next_member as it does not need to
re-skip 0 words on each iteration. (Likely bms_first_member() is not
needed anywhere in the code base)
int varno;
while ((varno = bms_first_member(result_relids)) >= 0)
remove_result_refs(root, varno, (Node *) f);
can also make the loop condition > 0, rather than >= 0 to save the
final futile attempt at finding a value that'll never be there.
2. The following comment seems to indicate that we can go ahead and
make parallelise the result processing, but the code still defers to
the checks below and may still end up not parallelising if say,
there's a non-parallel safe function call in the SELECT's target list.
case RTE_RESULT:
/* Sure, execute it in a worker if you want. */
break;
3. You may as well just ditch the variable and just do:
Assert(rel->relid > 0);
Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_RESULT);
instead of:
RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
/* Should only be applied to RTE_RESULT base relations */
Assert(rel->relid > 0);
rte = planner_rt_fetch(rel->relid, root);
Assert(rte->rtekind == RTE_RESULT);
There are a few other cases doing just that in costsize.c
4. I don't quite understand why this changed in join.out
@@ -3596,7 +3588,7 @@ select t1.* from
Hash Right Join
Output: i8.q2
Hash Cond: ((NULL::integer) = i8b1.q2)
- -> Hash Left Join
+ -> Hash Join
Can you explain why that's valid? I understand this normally occurs
when a qual exists which would filter out the NULL rows produced by
the join, but I don't see that in this case.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
I've just looked over the v4 patch. I agree that having an RTE for a
from-less SELECT seems like a good idea.
Thanks for looking!
While reading the patch, I noted the following:
1. It's more efficient to use bms_next_member as it does not need to
re-skip 0 words on each iteration. (Likely bms_first_member() is not
needed anywhere in the code base)
Sure. As the comment says, this isn't meant to be super efficient for
multiple removable RTEs, but we might as well use the other API.
2. The following comment seems to indicate that we can go ahead and
make parallelise the result processing, but the code still defers to
the checks below and may still end up not parallelising if say,
there's a non-parallel safe function call in the SELECT's target list.
Adjusted the comment.
3. You may as well just ditch the variable and just do:
Assert(rel->relid > 0);
Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_RESULT);
instead of:
RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
There are a few other cases doing just that in costsize.c
Meh. I'm not really on board with doing it that way, as it'll just
mean more to change if the code is ever modified to look at other
fields of the RTE. Still, I see your point that other places in
costsize.c are doing it without PG_USED_FOR_ASSERTS_ONLY, so changed.
4. I don't quite understand why this changed in join.out
@@ -3596,7 +3588,7 @@ select t1.* from
Hash Right Join
Output: i8.q2 Hash Cond: ((NULL::integer) = i8b1.q2) - -> Hash Left Join + -> Hash Join
Can you explain why that's valid?
Excellent question. The reason that plan changed is the logic I added
in find_nonnullable_rels_walker:
+ * If the PHV's syntactic scope is exactly one rel, it will be forced
+ * to be evaluated at that rel, and so it will behave like a Var of
+ * that rel: if the rel's entire output goes to null, so will the PHV.
In this case there's a PHV wrapped around b2.d2, and this change allows
reduce_outer_joins_pass2 to detect that the i8-to-b2 left join can
be simplified to an inner join because the qual just above it (on the
left join of b1 to i8/b2) is strict for b2; that is, the condition
(b2.d2 = b1.q2) cannot succeed for a null-extended i8/b2 row.
If you reverse out just that change you'll see why I added it: without it,
the plan for the earlier "test a corner case in which we shouldn't apply
the star-schema optimization" isn't optimized as much as I thought it
should be.
v5 attached; this responds to your comments plus Alexander's earlier
gripe about not getting a clean build with --disable-cassert.
No really substantive changes though.
regards, tom lane
Attachments:
get-rid-of-empty-jointrees-5.patchtext/x-diff; charset=us-ascii; name=get-rid-of-empty-jointrees-5.patchDownload+1069-428
I wrote:
If you reverse out just that change you'll see why I added it: without it,
the plan for the earlier "test a corner case in which we shouldn't apply
the star-schema optimization" isn't optimized as much as I thought it
should be.
Hmm ... looking at this closer, it seems like this patch probably breaks
what that regression test case was actually meant to test, ie once we've
deleted the VALUES subselects from the jointree, it's likely that the
planner join-ordering mistake that was testing for can no longer happen,
because the join just plain doesn't exist.
I'll plan to deal with that by running the test case with actual small
tables instead of VALUES subselects. It might be useful to run the test
case in its current form as an exercise for the RTE_RESULT optimizations,
but that would be separate from its current intention.
regards, tom lane
On Sat, 5 Jan 2019 at 08:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:
v5 attached; this responds to your comments plus Alexander's earlier
gripe about not getting a clean build with --disable-cassert.
No really substantive changes though.
I ran a few benchmarks on an AWS m5d.large instance based on top of
c5c7fa261f5. The biggest regression I see is from a simple SELECT 1 at
around 5-6%. A repeat of your test of SELECT 2+2 showed about half
that regression so the simple addition function call is introducing
enough overhead to lower the slowdown percentage by a good amount.
Test 3 improved performance a bit.
SELECT 1; I believe is a common query for some connection poolers as a
sort of ping to the database. In light of that, the performance drop
of 2 microseconds per query is not going to amount to very much in
total for that use case. i.e you'll need to do half a million pings
before it'll cost you 1 second of additional CPU time.
Results and tests are:
Setup: create table t1 (id int primary key);
Test 1: explain select 1;
Unpatched:
$ pgbench -n -f bench1.sql -T 60 postgres
tps = 30899.599603 (excluding connections establishing)
tps = 30806.247429 (excluding connections establishing)
tps = 30330.971411 (excluding connections establishing)
Patched:
tps = 28971.551297 (excluding connections establishing)
tps = 28892.053072 (excluding connections establishing)
tps = 28881.105928 (excluding connections establishing)
(5.75% drop)
Test 2: explain select * from t1 inner join (select 1 as x) x on t1.id=x.x;
Unpatched:
$ pgbench -n -f bench2.sql -T 60 postgres
tps = 14340.027655 (excluding connections establishing)
tps = 14392.871399 (excluding connections establishing)
tps = 14335.615020 (excluding connections establishing)
Patched:
tps = 14269.714239 (excluding connections establishing)
tps = 14305.901601 (excluding connections establishing)
tps = 14261.319313 (excluding connections establishing)
(0.54% drop)
Test 3: explain select * from t1 left join (select 1 as x) x on t1.id=x.x;
Unpatched:
$ pgbench -n -f bench3.sql -T 60 postgres
tps = 11404.769545 (excluding connections establishing)
tps = 11477.229511 (excluding connections establishing)
tps = 11365.426342 (excluding connections establishing)
Patched:
tps = 11624.081759 (excluding connections establishing)
tps = 11649.150950 (excluding connections establishing)
tps = 11571.724571 (excluding connections establishing)
(1.74% gain)
Test 4: explain select * from t1 inner join (select * from t1) t2 on
t1.id=t2.id;
Unpatched:
$ pgbench -n -f bench4.sql -T 60 postgres
tps = 9966.796818 (excluding connections establishing)
tps = 9887.775388 (excluding connections establishing)
tps = 9906.681296 (excluding connections establishing)
Patched:
tps = 9845.451081 (excluding connections establishing)
tps = 9936.377521 (excluding connections establishing)
tps = 9915.724816 (excluding connections establishing)
(0.21% drop)
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
I ran a few benchmarks on an AWS m5d.large instance based on top of
c5c7fa261f5. The biggest regression I see is from a simple SELECT 1 at
around 5-6%. A repeat of your test of SELECT 2+2 showed about half
that regression so the simple addition function call is introducing
enough overhead to lower the slowdown percentage by a good amount.
I can reproduce a small slowdown on "SELECT 1;", though for me it's
circa 2% not 5-6%. I'm not entirely sure that's above the noise level
--- I tend to see variations of that size even from unrelated code
changes. But to the extent that it's real, it seems like it must be
coming from one of these places:
* replace_empty_jointree adds a few pallocs for the new RTE and
jointree entry.
* After subquery_planner calls replace_empty_jointree, subsequent
places that loop over the rtable will see one entry instead of none.
They won't do much of anything with it, but it's a few more cycles.
* remove_useless_result_rtes is new code; it won't do much in this
case either, but it still has to examine the jointree.
* query_planner() does slightly more work before reaching its fast-path
exit.
None of these are exactly large costs, and it's hard to get rid of
any of them without ugly code contortions. I experimented with
micro-optimizing the trivial case in remove_useless_result_rtes,
but it didn't seem to make much difference for "SELECT 1", and it
would add cycles uselessly in all larger queries.
I also noticed that I'd been lazy in adding RTE_RESULT support to
build_simple_rel: we can save a couple of palloc's if we give it its own
code path. That did seem to reduce the penalty a shade, though I'm
still not sure that it's above the noise level.
SELECT 1; I believe is a common query for some connection poolers as a
sort of ping to the database. In light of that, the performance drop
of 2 microseconds per query is not going to amount to very much in
total for that use case. i.e you'll need to do half a million pings
before it'll cost you 1 second of additional CPU time.
Yeah, I agree this is not something to get hot & bothered over, but
I thought it was worth spending an hour seeing if there were any
easy wins. Not much luck.
Anyway, herewith v6, rebased up to HEAD, with the build_simple_rel
improvement and the regression test fix I mentioned earlier.
regards, tom lane