Re: [HACKERS] Add Roman numeral conversion to to_number

[Douglas Doole]

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

[Douglas Doole]

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

[Douglas Doole]

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 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

[Douglas Doole]

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

[Douglas Doole]

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

[Douglas Doole]

>
> 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

[Douglas Doole]

>
> 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

[Douglas Doole]

>
> 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

[Douglas Doole]

>
> 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

[Douglas Doole]

>
> 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
+ 		

Re: [HACKERS] [PATCH] Push limit to sort through a subquery

[Douglas Doole]

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 <robertmh...@gmail.com> wrote:

> On Thu, Aug 17, 2017 at 11:36 AM, Douglas Doole <dougdo...@gmail.com>
> 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,

Re: [HACKERS] [PATCH] Push limit to sort through a subquery

[Douglas Doole]

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()

[Douglas Doole]

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

[Douglas Doole]

>
> 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

[Douglas Doole]

>
> 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

[Douglas Doole]

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 <dougdo...@gmail.com>
> 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

[Douglas Doole]

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

[Douglas Doole]

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 <t...@sss.pgh.pa.us> wrote:

> Douglas Doole <dougdo...@gmail.com> 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

[Douglas Doole]

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 = 

Re: [HACKERS] WIP: Faster Expression Processing v4

[Douglas Doole]

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

[Douglas Doole]

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

[Douglas Doole]

>
> 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

[Douglas Doole]

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