Re: [HACKERS] Add Roman numeral conversion to to_number
I finally figured out why docs weren't building on my machine so I was able to take a look at your doc patch too. I think it's fine. Reading it did suggest a couple extra negative tests to confirm that when 'rn' is specified, all other elements are ignored: select to_number('vii7', 'rn9'); select to_number('7vii', '9rn');
Re: [HACKERS] Add Roman numeral conversion to to_number
Oliver, I took a look at your tests and they look thorough to me. One recommendation, instead of having 3999 separate selects to test every legal roman numeral, why not just do something like this: do $$ declare i int; rn text; rn_val int; begin for i in 1..3999 loop rn := trim(to_char(i, 'rn')); rn_val := to_number(rn, 'rn'); if (i <> rn_val) then raise notice 'Mismatch: i=% rn=% rn_val=%', i, rn, rn_val; end if; end loop; raise notice 'Tested roman numerals 1..3999'; end; $$; It's a lot easier to maintain than separate selects.
Re: [HACKERS] Add Roman numeral conversion to to_number
Ah. Sorry I missed them - I'll give them a look. (Won't be able to get to it until Saturday though.) On Thu, Sep 14, 2017 at 10:06 PM Oliver Ford wrote: > I'll fix the brace, but there are two other patches in the first email for > tests and docs. For some reason the commitfest app didn't pick them up. > > On Friday, 15 September 2017, Doug Doole wrote: > >> The following review has been posted through the commitfest application: >> make installcheck-world: tested, passed >> Implements feature: tested, passed >> Spec compliant: not tested >> Documentation:not tested >> >> Code looks fine, but one niggly complaint at line 146 of the patch file >> ("while (*cp) {"). A K&R style brace slipped in, which doesn't match the >> formatting of the file. >> >> Given that this is providing new formatting options, there should be new >> tests added that validate the options and error handling. >> >> There's also the "do we want this?" debate from the discussion thread >> that still needs to be resolved. (I don't have an opinion either way.) >> >> I'm sending this back to the author to address the first two issues. >> >> The new status of this patch is: Waiting on Author >> >> -- >> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) >> To make changes to your subscription: >> http://www.postgresql.org/mailpref/pgsql-hackers >> >
Re: [HACKERS] Trouble with amcheck
Thanks all. Making and installing the contribs got me rolling again. (I tried "make world" but ran into trouble with the XML docs. But that's pain and suffering for another day.) I'd agree that "make installcheck-world" should imply that all prereqs are met - that's certainsly the normal behaviour for make. - Doug
[HACKERS] Trouble with amcheck
I just cloned PostgreSQL to a new machine today (Ubuntu 17.04). "make install" and "make check-world" run fine but "make installcheck-world" is having trouble with amcheck: In contrib/amcheck/results: CREATE EXTENSION amcheck; ERROR: could not open extension control file "/home/doole/pgCommunity/install/share/postgresql/extension/amcheck.control": No such file or directory I expect I'm missing something in the machine set up, but I'm stumped as to what. Any suggestions? Thanks - Doug
Re: [HACKERS] [PATCH] Push limit to sort through a subquery
> > No. You can't easily avoid recursion for the merge-append case, since > that has to descend to multiple children. Agreed. That's why it's not in the while loop in my sketch of the suggested rework. > TBH I dislike the fact that > you did the subquery case randomly differently from the existing cases; > it should just have been added as an additional recursive case. Since > this is done only once at query startup, worrying about hypothetical > micro-performance issues seems rather misguided. > Habit mostly - why write code with potential performance problems when the alternative is just as easy to read? I disagree with saying "it's just done at query startup so don't worry about it too much". Back in my days with DB2 we were working on the TPCC benchmark (when it was still relevant) and we found that the startup cost was a huge factor when doing rapid fire, cached, simple queries. In many cases the startup cost was >50% of the overall query execution time. Our testing here in Salesforce is showing a similar pattern in some of our workloads and PostgreSQL. I'm not trying to argue that this particular issue of recurse/don't recurse is going to make or break anything. It's just where my head's at when I look at the code. - Doug Salesforce
Re: [HACKERS] [PATCH] Push limit to sort through a subquery
> > I don't greatly like the way that the regression test case filters > the output; it hides too much IMO. I'd be inclined to try to return > the EXPLAIN output with as much detail preserved as possible. Maybe > we could use regex substitution on lines of the output to get rid of > stuff that won't be invariant. > My first thought was to do a regex over the explain output to mask the troublesome value, but I couldn't figure out how to make it work and didn't find an example (admittedly didn't spent a huge amount of time hunting though). I'd love to see how to put it together. Doug - Salesforce
Re: [HACKERS] [PATCH] Push limit to sort through a subquery
> > Here's a not-really-tested draft patch for that. > I don't know the parallel code so I'm not going to comment on the overall patch, but a thought on ExecSetTupleBound(). That function is getting a number of cases where we're doing recursion for a single child (result, gather, gather-merge). Recursion for a single child isn't particularly efficient. I know that if there's a single case of recursion like this, compilers will frequently turn it into a loop, but I don't know if compilers can optimize a branched case like this. Would we be better off moving those cases into the while loop I added to avoid the recursion? So we'd end up with something like: while () { if subquery else if result else if gather else if gather merge } if sort else if merge append And a nit from my original fix now that I've looked at the pg10 code more. The two casts I added (to SubqueryScanState and Node) should probably be changed to castNode() calls. - Doug Salesforce
Re: [HACKERS] [PATCH] Push limit to sort through a subquery
> > Buildfarm members with force_parallel_mode=regress are failing now. I > haven't had a chance to investigate exactly what's going on here, but > I think there are probably several issues: > Not an auspicious beginning for my first patch :-( > 3. However, even if it did that, it would only affect the leader, not > the workers, because each worker will of course have a separate copy > of each executor state node. We could fix that by having the Gather > or Gather Merge node also store the bound and propagate that to each > worker via DSM, and then have each worker call pass_down_bound itself. > (This would require a bit of refactoring of the API for > pass_down_bound, but it looks doable.) > >From previous experience, pushing the limit to the workers has the potential to be a big win . In the short run, I'm not sure we have a better alternative than > removing this test - unless somebody has a better idea? - but it would > be good to work on all of the above problems. > I haven't played with parallel mode at all yet. Is it possible to disable force_parallel_mode for the new test? If not, then I'd agree that removing the test is probably the only reasonable short term fix. - Doug Salesforce
Re: [HACKERS] [PATCH] Push limit to sort through a subquery
> > 1. The header comment for pass_down_bound() could mention "one or more > levels of subqueries" rather than "a subquery". > Fixed 2. The first of the comments in the function body appears to have a > whitespace issue that needs to be fixed manually or, better yet, > addressed by pgindent. > Fixed > 3. The formatting of the comment in the regression tests appears to be > unlike any other comment in that same file. > A side effect of inheriting it from our branches ;-) Reworked. > 4. I am pretty doubtful that "Memory: 25kB" is going to be stable > enough for us to want that output memorialized in the regression ... > Fair enough. I wanted to be a bit more sophisticated in my check than looking for a single value so I worked out something that distills the explain down to the key elements. *** a/src/backend/executor/nodeLimit.c --- b/src/backend/executor/nodeLimit.c *** *** 308,313 recompute_limits(LimitState *node) --- 308,316 * since the MergeAppend surely need read no more than that many tuples from * any one input. We also have to be prepared to look through a Result, * since the planner might stick one atop MergeAppend for projection purposes. + * We can also accept one or more levels of subqueries that have no quals or + * SRFs (that is, each subquery is just projecting columns) between the LIMIT + * and any of the above. * * This is a bit of a kluge, but we don't have any more-abstract way of * communicating between the two nodes; and it doesn't seem worth trying *** *** 320,325 recompute_limits(LimitState *node) --- 323,349 static void pass_down_bound(LimitState *node, PlanState *child_node) { + /* + * If the child is a subquery that does no filtering (no predicates) + * and does not have any SRFs in the target list then we can potentially + * push the limit through the subquery. It is possible that we could have + * multiple subqueries, so tunnel through them all. + */ + while (IsA(child_node, SubqueryScanState)) + { + SubqueryScanState *subqueryScanState = (SubqueryScanState *) child_node; + + /* + * Non-empty predicates or an SRF means we cannot push down the limit. + */ + if (subqueryScanState->ss.ps.qual != NULL || + expression_returns_set((Node *) child_node->plan->targetlist)) + return; + + /* Use the child in the following checks */ + child_node = subqueryScanState->subplan; + } + if (IsA(child_node, SortState)) { SortState *sortState = (SortState *) child_node; *** a/src/test/regress/expected/subselect.out --- b/src/test/regress/expected/subselect.out *** *** 1041,1043 NOTICE: x = 9, y = 13 --- 1041,1095 (3 rows) drop function tattle(x int, y int); + -- + -- Test that LIMIT can be pushed to SORT through a subquery that just + -- projects columns + -- + create table sq_limit (pk int primary key, c1 int, c2 int); + insert into sq_limit values + (1, 1, 1), + (2, 2, 2), + (3, 3, 3), + (4, 4, 4), + (5, 1, 1), + (6, 2, 2), + (7, 3, 3), + (8, 4, 4); + -- The explain contains data that may not be invariant, so + -- filter for just the interesting bits. The goal here is that + -- we should see three notices, in order: + -- NOTICE: Limit + -- NOTICE: Subquery + -- NOTICE: Top-N Sort + -- A missing step, or steps out of order means we have a problem. + do $$ + declare x text; + begin + for x in + explain (analyze, summary off, timing off, costs off) + select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3 + loop + if (left(ltrim(x), 5) = 'Limit') then + raise notice 'Limit'; + end if; + if (left(ltrim(x), 12) = '-> Subquery') then + raise notice 'Subquery'; + end if; + if (left(ltrim(x), 18) = 'Sort Method: top-N') then + raise notice 'Top-N Sort'; + end if; + end loop; + end; + $$; + NOTICE: Limit + NOTICE: Subquery + NOTICE: Top-N Sort + select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; + pk | c2 + + + 1 | 1 + 5 | 1 + 2 | 2 + (3 rows) + + drop table sq_limit; *** a/src/test/regress/sql/subselect.sql --- b/src/test/regress/sql/subselect.sql *** *** 540,542 select * from --- 540,588 where tattle(x, u); drop function tattle(x int, y int); + + -- + -- Test that LIMIT can be pushed to SORT through a subquery that just + -- projects columns + -- + create table sq_limit (pk int primary key, c1 int, c2 int); + insert into sq_limit values + (1, 1, 1), + (2, 2, 2), + (3, 3, 3), + (4, 4, 4), + (5, 1, 1), + (6, 2, 2), + (7, 3, 3), + (8, 4, 4); + + -- The explain contains data that may not be invariant, so + -- filter for just the interesting bits. The goal here is that + -- we should see three notices, in order: + -- NOTICE: Limit + -- NOTICE: Subquery + -- NOTICE: Top-N Sort + -- A missing step, or steps out of order means we have a problem. + do $$ + declare x text; + begin + for
Re: [HACKERS] [PATCH] Push limit to sort through a subquery
Thanks for the feedback on my original patch Robert. Here's an updated patch that will tunnel through multiple SubqueryScanStates. - Doug Salesforce On Thu, Aug 17, 2017 at 6:33 PM Robert Haas wrote: > On Thu, Aug 17, 2017 at 11:36 AM, Douglas Doole > wrote: > >> I completely agree. The further a limit can be pushed down, the better. >> >> The patch looks good to me. >> > > It seems like a somewhat ad-hoc approach; it supposes that we can take any > query produced by deparseSelectStmtForRel() and stick a LIMIT clause onto > the very end and all will be well. Maybe that's not a problematic > assumption, not sure. The grammar happens to allow both FOR UPDATE LIMIT n > and LIMIT n FOR UPDATE even though only the latter syntax is documented. > > Regarding the other patch on this thread, you mentioned upthread that "If > it is possible to get more than one SubqueryScanState and/or ResultState > between the limit and sort, then the first block of code could be placed in > a while loop." I think that's not possible for a ResultState, but I think > it *is* possible for a SubqueryScanState. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > *** a/src/backend/executor/nodeLimit.c --- b/src/backend/executor/nodeLimit.c *** *** 308,313 recompute_limits(LimitState *node) --- 308,315 * since the MergeAppend surely need read no more than that many tuples from * any one input. We also have to be prepared to look through a Result, * since the planner might stick one atop MergeAppend for projection purposes. + * We can also accept a subquery that has no quals or SRFs (that is, the + * subquery is just projecting columns) between the LIMIT and any of the above. * * This is a bit of a kluge, but we don't have any more-abstract way of * communicating between the two nodes; and it doesn't seem worth trying *** *** 320,325 recompute_limits(LimitState *node) --- 322,348 static void pass_down_bound(LimitState *node, PlanState *child_node) { + /* + * If the child is a subquery that does no filtering (no predicates) + * and does not have any SRFs in the target list then we can potentially + * push the limit through the subquery. It is possible that we could have + * multiple subqueries, so tunnel through them all. + */ + while (IsA(child_node, SubqueryScanState)) + { + SubqueryScanState *subqueryScanState = (SubqueryScanState *) child_node; + + /* + * Non-empty predicates or an SRF means we cannot push down the limit. + */ + if (subqueryScanState->ss.ps.qual != NULL || + expression_returns_set((Node *) child_node->plan->targetlist)) + return; + + /* Use the child in the following checks */ + child_node = subqueryScanState->subplan; + } + if (IsA(child_node, SortState)) { SortState *sortState = (SortState *) child_node; *** a/src/test/regress/expected/subselect.out --- b/src/test/regress/expected/subselect.out *** *** 1041,1043 NOTICE: x = 9, y = 13 --- 1041,1077 (3 rows) drop function tattle(x int, y int); + - + --TEST LIMIT pushdown through subquery scan node + - + create table sq_limit (pk int primary key, c1 int, c2 int); + insert into sq_limit values + (1, 1, 1), + (2, 2, 2), + (3, 3, 3), + (4, 4, 4), + (5, 1, 1), + (6, 2, 2), + (7, 3, 3), + (8, 4, 4); + explain (analyze, summary off, timing off, costs off) + select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; +QUERY PLAN + + Limit (actual rows=3 loops=1) +-> Subquery Scan on x (actual rows=3 loops=1) + -> Sort (actual rows=3 loops=1) +Sort Key: sq_limit.c1, sq_limit.pk +Sort Method: top-N heapsort Memory: 25kB +-> Seq Scan on sq_limit (actual rows=8 loops=1) + (6 rows) + + select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; + pk | c2 + + + 1 | 1 + 5 | 1 + 2 | 2 + (3 rows) + + drop table sq_limit; *** a/src/test/regress/sql/subselect.sql --- b/src/test/regress/sql/subselect.sql *** *** 540,542 select * from --- 540,563 where tattle(x, u); drop function tattle(x int, y int); + + - + --TEST LIMIT pushdown through subquery scan node + - + create table sq_limit (pk int primary key, c1 int, c2 int); + insert into sq_limit values + (1, 1, 1), + (2, 2, 2), + (3, 3, 3), + (4, 4, 4), + (5, 1, 1), + (6, 2, 2), + (7
Re: [HACKERS] [PATCH] Push limit to sort through a subquery
I completely agree. The further a limit can be pushed down, the better. The patch looks good to me. On Thu, Aug 17, 2017 at 8:27 AM Konstantin Knizhnik < k.knizh...@postgrespro.ru> wrote: > > > On 29.04.2017 00:13, Douglas Doole wrote: > > If you add this to the commitfest app, more people might look at it when >> the next commitfest opens. > > > I have added it. https://commitfest.postgresql.org/14/1119/ > > Also, it might help if you can provide a query/ies with numbers where this >> optimization shows improvement. >> > > I can't provide the real queries where we encountered the problem because > they are internal. However I showed a simplified version of the queries in > my first post. > > On our queries, the change made quite a difference - execution time > dropped from 31.4 seconds to 7.2 seconds. Explain analyze also shows that > memory use dropped significantly and we didn't have to spill the sort to > disk > > From: > > -> Sort (cost=989.95..1013.27 rows=9326 width=30) > (node_startup_time/loop=31328.891, node_total_time/loop: 31329.756 > rows=2001 loops=1) Buffers: temp read=772 written=11201 lsm_bufmgr > hits=3392 Sort Key: *** Sort Method: external merge Sort Space Used: 89592 > Sort Space Type: Disk > > To: > > -> Sort (cost=989.95..1013.27 rows=9326 width=30) > (node_startup_time/loop=7123.275, node_total_time/loop: 7123.504 rows=2001 > loops=1) Buffers: lsm_bufmgr hits=3387 Sort Key: *** Sort Method: top-N > heapsort Sort Space Used: 3256 Sort Space Type: Memory > > > Attached please find yet another small patch which pushes down LIMIT to > ForeignScan. > I should notice that currently Postgres optimizer is using "Merge Append" > and fetches from remote nodes only required number of tuples. > So even without LIMIT push down, postgres_fdw will not pull the whole > table from remote host. > postgres_fdw is using cursor for fetching data from remote. Default fetch > size is 100, so even without limit remote query will fetch no more than > 100 rows at remote site. > > Assume the following example: > > postgres=# create extension postgres_fdw; > postgres=# create server shard1 FOREIGN DATA WRAPPER postgres_fdw > options(dbname 'postgres', host 'localhost', port '5432'); > postgres=# create server shard2 FOREIGN DATA WRAPPER postgres_fdw > options(dbname 'postgres', host 'localhost', port '5432'); > postgres=# CREATE USER MAPPING for $user SERVER shard1 options (user > '$user'); > postgres=# CREATE USER MAPPING for $user SERVER shard1 options (user > '$user'); > postgres=# CREATE TABLE t(u integer primary key, v integer); > postgres=# CREATE TABLE t1(u integer primary key, v integer); > postgres=# CREATE TABLE t2(u integer primary key, v integer); > postgres=# insert into t1 values (generate_series(1,10), > random()*10); > postgres=# insert into t2 values (generate_series(1,10), > random()*10); > postgres=# CREATE FOREIGN TABLE t_fdw1() inherits (t) server shard1 > options(table_name 't1'); > postgres=# CREATE FOREIGN TABLE t_fdw2() inherits (t) server shard2 > options(table_name 't2'); > > > postgres=# explain analyze select * from t order by u limit 1; > QUERY > PLAN > > --- > Limit (cost=200.15..200.20 rows=1 width=8) (actual time=2.010..2.010 > rows=1 loops=1) >-> Merge Append (cost=200.15..449.39 rows=5121 width=8) (actual > time=2.009..2.009 rows=1 loops=1) > Sort Key: t.u > -> Index Scan using t_pkey on t (cost=0.12..8.14 rows=1 > width=8) (actual time=0.005..0.005 rows=0 loops=1) > -> Foreign Scan on t_fdw2 (cost=100.00..193.92 rows=2560 > width=8) (actual time=1.074..1.074 rows=1 loops=1) > -> Foreign Scan on t_fdw1 (cost=100.00..193.92 rows=2560 > width=8) (actual time=0.928..0.928 rows=1 loops=1) > Planning time: 0.769 ms > Execution time: 6.837 ms > (8 rows) > > As you can see foreign scan fetches only one row from each remote node. > > But still pushing down limit can have positive effect on performance, > especially if SORT can be replaced with TOP-N. > I got the following results (time in seconds): > > Query > original > limit push down > select * from t order by u limit 1 > 2.276 > 1.777 > select * from t order by v limit 1 > 100 42 > > There is index for "u", so fetching records with smallest "u" values can > be done without sorting, so times are similar. > But in case of sorting by "v", pushing down limit allows to use TOP-1 > instead of global sort and it reduces query execution time more than 2 > times. > > -- > > Konstantin Knizhnik > Postgres Professional: http://www.postgrespro.com > The Russian Postgres Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
[HACKERS] Inconsistencies in from_char_parse_int_len()
I was playing with TO_TIMESTAMP() and I noticed a weird result: postgres=# select to_timestamp('20170-07-24 21:59:57.12345678', '-mm-dd hh24:mi:ss.us'); to_timestamp 20170-07-24 22:00:09.345678+00 (1 row) Even though the "us" token is supposed to be restricted to 00-99 it looks like the microseconds was calculated as 12.345678. Digging into the code, I found inconsistencies in from_char_parse_int_len(). From formatting.c: !/* ! * Read a single integer from the source string, into the int pointed to by ! * 'dest'. If 'dest' is NULL, the result is discarded. ! * ! * In fixed-width mode (the node does not have the FM suffix), consume at most ! * 'len' characters. However, any leading whitespace isn't counted in 'len'. ! * !static int !from_char_parse_int_len(int *dest, char **src, const int len, FormatNode *node) !{ !if (S_FM(node->suffix) || is_next_separator(node)) !{ !/* ! * This node is in Fill Mode, or the next node is known to be a ! * non-digit value, so we just slurp as many characters as we can get. ! */ !errno = 0; !result = strtol(init, src, 10); !} !else !{ !/* ! * We need to pull exactly the number of characters given in 'len' out ! * of the string, and convert those. ! */ !char *last; ! !if (used < len) !ereport(ERROR, !(errcode(ERRCODE_INVALID_DATETIME_FORMAT), So the function prologue disagrees with the code. In the first condition strtol() can consume more than 'len' digits. In the else, we error out if we don't have exactly 'len' characters. What's the proper behaviour here? - Doug Salesforce
Re: [HACKERS] Cached plans and statement generalization
> > One interesting idea from Doug Doole was to do it between the tokenizer > and parser. I think they are glued together so you would need a way to run > the tokenizer separately and compare that to the tokens you stored for the > cached plan. > When I did this, we had the same problem that the tokenizer and parser were tightly coupled. Fortunately, I was able to do as you suggest and run the tokenizer separately to do my analysis. So my model was to do statement generalization before entering the compiler at all. I would tokenize the statement to find the literals and generate a new statement string with placeholders. The new string would the be passed to the compiler which would then tokenize and parse the reworked statement. This means we incurred the cost of tokenizing twice, but the tokenizer was lightweight enough that it wasn't a problem. In exchange I was able to do statement generalization without touching the compiler - the compiler saw the generalized statement text as any other statement and handled it in the exact same way. (There was just a bit of new code around variable binding.)
Re: [HACKERS] [PATCH] Push limit to sort through a subquery
> > If you add this to the commitfest app, more people might look at it when > the next commitfest opens. I have added it. https://commitfest.postgresql.org/14/1119/ Also, it might help if you can provide a query/ies with numbers where this > optimization shows improvement. > I can't provide the real queries where we encountered the problem because they are internal. However I showed a simplified version of the queries in my first post. On our queries, the change made quite a difference - execution time dropped from 31.4 seconds to 7.2 seconds. Explain analyze also shows that memory use dropped significantly and we didn't have to spill the sort to disk From: -> Sort (cost=989.95..1013.27 rows=9326 width=30) (node_startup_time/loop=31328.891, node_total_time/loop: 31329.756 rows=2001 loops=1) Buffers: temp read=772 written=11201 lsm_bufmgr hits=3392 Sort Key: *** Sort Method: external merge Sort Space Used: 89592 Sort Space Type: Disk To: -> Sort (cost=989.95..1013.27 rows=9326 width=30) (node_startup_time/loop=7123.275, node_total_time/loop: 7123.504 rows=2001 loops=1) Buffers: lsm_bufmgr hits=3387 Sort Key: *** Sort Method: top-N heapsort Sort Space Used: 3256 Sort Space Type: Memory
Re: [HACKERS] [PATCH] Push limit to sort through a subquery
Thanks for the feedback Ashutosh. I disagree that we need to call pass_down_bound() recursively. The whole point of the recursive call would be to tunnel through a plan node. Since SubqueryScanState only has one child (unlike MergeAppendState) this can be more efficiently done inline rather than via a recursive call. In the case of ResultState, this is classic tail-end recursion and the C compiler should optimize it out. As we add more recursive calls though, it becomes harder for the compiler to do the right thing. (I'll admit that I'm not up on what state-of-the-art in recursion removal though, so maybe my concern is moot. That said, I prefer not to depend on compiler optimizations if there's a clean alternative way to express the code.) I do agree that it would make the code cleaner to handle ResultState and SubqueryScanState in a similar fashion. My preference, however, would be to remove the recursive call from ResultState handling. That would give us code like: if child == SubqueryScanState child = SubqueryScanState->child else if child == ResultState child = ResultState->child if child == SortState limit pushdown else if child == MergeAppendState recursive call on all children If it is possible to get more than one SubqueryScanState and/or ResultState between the limit and sort, then the first block of code could be placed in a while loop. If you'd prefer that I rework the patch as I discussed, just let me know and I'll resubmit it. - Doug Salesforce On Wed, Apr 19, 2017 at 1:57 AM Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > The function pass_down_bound() is a recursive function. For > SubqueryScanState we have to do something similar to ResultState i.e. > call pass_down_bound() recursively on subqueryScanState->subplan. > > Please add this to the next commitfest. > > On Wed, Apr 19, 2017 at 6:09 AM, Douglas Doole > wrote: > > We've hit a case where pass_down_bound() isn't pushing the row count > limit > > from limit into sort. The issue is that we're getting a subquery scan > node > > between the limit and the sort. The subquery is only doing column > projection > > and has no quals or SRFs so it should be safe to push the limit into the > > sort. > > > > The query that hit the problem can be simplified to: > > > >SELECT * FROM (SELECT A,B FROM T ORDER BY C) LIMIT 5 > > > > (Yeah, the query's a little screwy in that the ORDER BY should really be > > outside the subselect, but it came from a query generator, so that's a > > different conversation.) > > > > Proposed patch is attached. > > > > - Doug > > Salesforce > > > > > > -- > > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > > To make changes to your subscription: > > http://www.postgresql.org/mailpref/pgsql-hackers > > > > > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company >
[HACKERS] [PATCH] Push limit to sort through a subquery
We've hit a case where pass_down_bound() isn't pushing the row count limit from limit into sort. The issue is that we're getting a subquery scan node between the limit and the sort. The subquery is only doing column projection and has no quals or SRFs so it should be safe to push the limit into the sort. The query that hit the problem can be simplified to: SELECT * FROM (SELECT A,B FROM T ORDER BY C) LIMIT 5 (Yeah, the query's a little screwy in that the ORDER BY should really be outside the subselect, but it came from a query generator, so that's a different conversation.) Proposed patch is attached. - Doug Salesforce *** a/src/backend/executor/nodeLimit.c --- b/src/backend/executor/nodeLimit.c *** *** 304,309 recompute_limits(LimitState *node) --- 304,311 * since the MergeAppend surely need read no more than that many tuples from * any one input. We also have to be prepared to look through a Result, * since the planner might stick one atop MergeAppend for projection purposes. + * We can also accept a subquery that has no quals or SRFs (that is, the + * subquery is just projecting columns) between the LIMIT and any of the above. * * This is a bit of a kluge, but we don't have any more-abstract way of * communicating between the two nodes; and it doesn't seem worth trying *** *** 316,321 recompute_limits(LimitState *node) --- 318,343 static void pass_down_bound(LimitState *node, PlanState *child_node) { + /* + * If the child is a subquery that does no filtering (no predicates) + * and does not have any SRFs in the target list then we can potentially + * push the limit through the subquery. + */ + if (IsA(child_node, SubqueryScanState)) + { + SubqueryScanState *subqueryScanState = (SubqueryScanState *) child_node; + + /* + * Non-empty predicates or an SRF means we cannot push down the limit. + */ + if (subqueryScanState->ss.ps.qual != NULL || + expression_returns_set((Node *) child_node->plan->targetlist)) + return; + + /* Use the child in the following checks */ + child_node = subqueryScanState->subplan; + } + if (IsA(child_node, SortState)) { SortState *sortState = (SortState *) child_node; *** a/src/test/regress/expected/subselect.out --- b/src/test/regress/expected/subselect.out *** *** 1041,1043 NOTICE: x = 9, y = 13 --- 1041,1077 (3 rows) drop function tattle(x int, y int); + - + --TEST LIMIT pushdown through subquery scan node + - + create table sq_limit (pk int primary key, c1 int, c2 int); + insert into sq_limit values + (1, 1, 1), + (2, 2, 2), + (3, 3, 3), + (4, 4, 4), + (5, 1, 1), + (6, 2, 2), + (7, 3, 3), + (8, 4, 4); + explain (analyze, summary off, timing off, costs off) + select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; +QUERY PLAN + + Limit (actual rows=3 loops=1) +-> Subquery Scan on x (actual rows=3 loops=1) + -> Sort (actual rows=3 loops=1) +Sort Key: sq_limit.c1, sq_limit.pk +Sort Method: top-N heapsort Memory: 25kB +-> Seq Scan on sq_limit (actual rows=8 loops=1) + (6 rows) + + select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; + pk | c2 + + + 1 | 1 + 5 | 1 + 2 | 2 + (3 rows) + + drop table sq_limit; *** a/src/test/regress/sql/subselect.sql --- b/src/test/regress/sql/subselect.sql *** *** 540,542 select * from --- 540,563 where tattle(x, u); drop function tattle(x int, y int); + + - + --TEST LIMIT pushdown through subquery scan node + - + create table sq_limit (pk int primary key, c1 int, c2 int); + insert into sq_limit values + (1, 1, 1), + (2, 2, 2), + (3, 3, 3), + (4, 4, 4), + (5, 1, 1), + (6, 2, 2), + (7, 3, 3), + (8, 4, 4); + + explain (analyze, summary off, timing off, costs off) + select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; + + select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; + + drop table sq_limit; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Table collision in join.sql and aggregates.sql
D'oh! The "temp" declaration had been removed from our test since we don't use temp tables. I missed that when applying it to the community code. You can ignore me now. On Fri, Mar 31, 2017 at 4:01 PM Tom Lane wrote: > Douglas Doole writes: > > As we've been merging our code with 9.6, a couple developers have had > > one-off failures in the join.sql and aggregates.sql test because the > tables > > T1, T2 and T3 have the wrong definitions. > > > Digging into it, I found that both files create the tables T1, T2, and T3 > > for a short period of time and then drop them. Since the > parallel_schedule > > file puts these two files into the same group, they can run concurrently. > > If it happens that the the two files hit the T1, T2, T3 tests at the same > > time, then you see the table definition problems. > > Hmmm ... that would indeed be a problem, except that aggregate.sql's > tables are temp tables, which should mean that they are in a schema > different from the one that join.sql is putting its tables in. Are you > sure you've identified the issue correctly? Because if this doesn't > work, there are an awful lot of similar hazards elsewhere in the > regression tests. > > regards, tom lane >
[HACKERS] Table collision in join.sql and aggregates.sql
As we've been merging our code with 9.6, a couple developers have had one-off failures in the join.sql and aggregates.sql test because the tables T1, T2 and T3 have the wrong definitions. Digging into it, I found that both files create the tables T1, T2, and T3 for a short period of time and then drop them. Since the parallel_schedule file puts these two files into the same group, they can run concurrently. If it happens that the the two files hit the T1, T2, T3 tests at the same time, then you see the table definition problems. I took the easy way of solving this and renamed the tables in aggregates.sql to AGG1, AGG2, and AGG3. (Picked on aggregates.sql since it had the T1, T2, T3 tests added in 9.6.) Doug - Salesforce *** a/src/test/regress/expected/aggregates.out --- b/src/test/regress/expected/aggregates.out *** *** 957,1023 LINE 1: select (select max(min(unique1)) from int8_tbl) from tenk1; -- -- Test removal of redundant GROUP BY columns -- ! create temp table t1 (a int, b int, c int, d int, primary key (a, b)); ! create temp table t2 (x int, y int, z int, primary key (x, y)); ! create temp table t3 (a int, b int, c int, primary key(a, b) deferrable); -- Non-primary-key columns can be removed from GROUP BY ! explain (costs off) select * from t1 group by a,b,c,d; ! QUERY PLAN ! -- HashAggregate Group Key: a, b !-> Seq Scan on t1 (3 rows) -- No removal can happen if the complete PK is not present in GROUP BY ! explain (costs off) select a,c from t1 group by a,c,d; ! QUERY PLAN ! -- HashAggregate Group Key: a, c, d !-> Seq Scan on t1 (3 rows) -- Test removal across multiple relations explain (costs off) select * ! from t1 inner join t2 on t1.a = t2.x and t1.b = t2.y ! group by t1.a,t1.b,t1.c,t1.d,t2.x,t2.y,t2.z; ! QUERY PLAN ! --- Group !Group Key: t1.a, t1.b, t2.x, t2.y -> Merge Join ! Merge Cond: ((t1.a = t2.x) AND (t1.b = t2.y)) ! -> Index Scan using t1_pkey on t1 ! -> Index Scan using t2_pkey on t2 (6 rows) ! -- Test case where t1 can be optimized but not t2 ! explain (costs off) select t1.*,t2.x,t2.z ! from t1 inner join t2 on t1.a = t2.x and t1.b = t2.y ! group by t1.a,t1.b,t1.c,t1.d,t2.x,t2.z; ! QUERY PLAN ! --- HashAggregate !Group Key: t1.a, t1.b, t2.x, t2.z -> Merge Join ! Merge Cond: ((t1.a = t2.x) AND (t1.b = t2.y)) ! -> Index Scan using t1_pkey on t1 ! -> Index Scan using t2_pkey on t2 (6 rows) -- Cannot optimize when PK is deferrable ! explain (costs off) select * from t3 group by a,b,c; ! QUERY PLAN ! -- HashAggregate Group Key: a, b, c !-> Seq Scan on t3 (3 rows) ! drop table t1; ! drop table t2; ! drop table t3; -- -- Test combinations of DISTINCT and/or ORDER BY -- --- 957,1023 -- -- Test removal of redundant GROUP BY columns -- ! create temp table agg1 (a int, b int, c int, d int, primary key (a, b)); ! create temp table agg2 (x int, y int, z int, primary key (x, y)); ! create temp table agg3 (a int, b int, c int, primary key(a, b) deferrable); -- Non-primary-key columns can be removed from GROUP BY ! explain (costs off) select * from agg1 group by a,b,c,d; !QUERY PLAN ! HashAggregate Group Key: a, b !-> Seq Scan on agg1 (3 rows) -- No removal can happen if the complete PK is not present in GROUP BY ! explain (costs off) select a,c from agg1 group by a,c,d; !QUERY PLAN ! HashAggregate Group Key: a, c, d !-> Seq Scan on agg1 (3 rows) -- Test removal across multiple relations explain (costs off) select * ! from agg1 inner join agg2 on agg1.a = agg2.x and agg1.b = agg2.y ! group by agg1.a,agg1.b,agg1.c,agg1.d,agg2.x,agg2.y,agg2.z; ! QUERY PLAN ! --- Group !Group Key: agg1.a, agg1.b, agg2.x, agg2.y -> Merge Join ! Merge Cond: ((agg1.a = agg2.x) AND (agg1.b = agg2.y)) ! -> Index Scan using agg1_pkey on agg1 ! -> Index Scan using agg2_pkey on agg2 (6 rows) ! -- Test case where agg1 can be optimized but not agg2 ! explain (costs off) select agg1.*,agg2.x,agg2.z ! from agg1 inner join agg2 on agg1.a = agg2.x and agg1.b = agg2.y ! group by agg1.a,agg1.b,agg1.c,agg1.d,agg2.x,agg2.z; ! QUERY PLAN ! --- HashAggregate !Group Key: agg1.a, agg1.b, agg2.x, agg2.z -> Merge Join ! Merge Cond: ((agg1.a = agg2.x
Re: [HACKERS] WIP: Faster Expression Processing v4
On Tue, Mar 14, 2017 at 3:16 PM Andres Freund wrote: > Hm. Right now ExprState's are allocated in several places - but we > could easily move that to a central place. Have a bit of a hard time > seing that that branch during *initialization* time is that expensive, > especially given that previously we allocated a lot of memory separately > too. > I didn't make any comparisons of the cost of the new init against the old init with this change in particular - I just saw that it made the new init faster. I also didn't play around to determine if the savings was found in removing the branch misprediction or inlining or both. I certainly wouldn't hold up your commit for this, but it's something that might be worth a second look once the dust has settled.
Re: [HACKERS] WIP: Faster Expression Processing v4
Andres, sorry I haven't had a chance to look at this great stuff you've been doing. I've wanted to get to it, but work keeps getting in the way. ;-) I do have one observation based on my experiments with your first version of the code. In my tests, I found that expression init becomes a lot more expensive in this new model. (That's neither a surprise, nor a concern.) In particular, the function ExprEvalPushStep() is quite hot. In my code I made the following changes: * Declare ExprEvalPushStep() "inline". * Remove the "if (es->steps_alloc == 0)" condition from ExprEvalPushStep(). * In ExecInitExpr(), add: state->steps_alloc = 16; state->steps = palloc(sizeof(ExprEvalStep) * es->steps_alloc); I found that this cut the cost of initializing the expression by about 20%. (Of course, that was on version 1 of your code, so the benefit may well be different now.) On Tue, Mar 14, 2017 at 11:51 AM Andres Freund wrote: > > Hmm. Could we make the instructions variable size? It would allow packing > > the small instructions even more tight, and we wouldn't need to obsess > over > > a particular maximum size for more complicated instructions. > > That makes jumps a lot more complicated. I'd experimented with it and > given it up as "not worth it". Back when I was at IBM, I spent a lot of time doing stuff like this. If you want to commit with the fixed size arrays, I'm happy to volunteer to look at packing it tighter as a follow-on piece of work. (It was already on my list of things to try anyhow.) > If we were to try to do so, we'd also > not like storing the pointer and enum variants both, since it'd again > would reduce the density. > >From my experience, it's worth the small loss in density to carry around both the pointer and the enum - it makes debugging so much easier. - Doug Salesforce
Re: [HACKERS] possible optimizations - pushing filter before aggregation
> > In above case wondering if we could do this > > Min(a) = 2 is the condition, generate condition *"a <= 2"* and push it > down as scan key. Since pushed down condition is lossy one for us ( it > gives values < 2), finally do a recheck of *"Min(a) = 2"*. > > For Max(a) = 2 we can have *"a >=2"*, > After replying, I was thinking along these lines too. I can't see any reason why it wouldn't work. The same would apply for HAVING clauses on min/max aggregations as well. For min, you should be able to pre-filter =, < , and <=. In all cases the pre-filter would be <=. For max it would be =, > , >= becoming >=. - Doug Doole Salesforce
Re: [HACKERS] possible optimizations - pushing filter before aggregation
On Fri, Nov 18, 2016 at 12:47 AM Pavel Stehule wrote: Isn't possible in this case push equivalence before aggregation? If I'm understanding you correctly, that would lead to wrong results. Here's a simple example: CREATE VIEW v AS SELECT MIN(a) m FROM t; and table T contains: T:A --- 1 2 3 SELECT * FROM v WHERE m = 2 The minimum value of A is 1, so the query should return no rows. However, if we filter first we'd be effectively doing the query: SELECT MIN(a) m FROM (SELECT a FROM t WHERE a=2) AS v(a) The subquery is going to return an intermediate result of: V:A --- 2 And the minimum of that is 2, which is the wrong answer. - Doug Salesforce