Date: Wed, 19 Feb 2020 20:16:36 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Melanie Plageman <melanieplageman@gmail.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Taylor Vesely <tvesely@pivotal.io>, Adam Lee <ali@pivotal.io>,
	Melanie Plageman <mplageman@pivotal.io>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Memory-Bounded Hash Aggregation
Message-ID: <20200219191636.gvdywx32kwbix6kd@development>
References: <617fea8258ba3df7bc8b21cd7f3afc219f2383d9.camel@j-davis.com>
 <20190802152123.ichmacgx3y3x7dc3@development>
 <CAFaX_4Ls5UGQ2UDYkWhn2xFq=V6BKpUbKSQnWmy27VWhQ2=enA@mail.gmail.com>
 <e52707d4665e32867fb6dd181825ef15f853b8bf.camel@j-davis.com>
 <20191214173225.3kzsbncr6brpxdll@development>
 <d72e494db7b075ef02c3163e6614e698701d2c3e.camel@j-davis.com>
 <775d754c-9a61-5363-6998-f87d15f1b592@iki.fi>
 <CAAKRu_aefEsv+UkQWqu+ioEnoiL2LJu9Diuh9BR8MbyXuZ0j4A@mail.gmail.com>
 <20200218185749.srk733n4z57lzxf2@development>
 <7612ea4d636b6341d0606f4e852d584798e04901.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="hbghtqe47acw4lbv"
Content-Disposition: inline
In-Reply-To: <7612ea4d636b6341d0606f4e852d584798e04901.camel@j-davis.com>
Precedence: bulk


--hbghtqe47acw4lbv
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline

Hi,

I've started reviewing the 20200218 version of the patch. In general it
seems fine, but I have a couple minor comments and two crashes.


1) explain.c currently does this:

I wonder if we could show something for plain explain (without analyze).
At least the initial estimate of partitions, etc. I know not showing
those details until after execution is what e.g. sort does, but I find
it a bit annoying.

A related comment is that maybe this should report also the initial
number of partitions, not just the total number. With just the total
it's impossible to say if there were any repartitions, etc.


2) The ExecBuildAggTrans comment should probably explain "spilled".


3) I wonder if we need to invent new opcodes? Wouldn't it be simpler to
just add a new flag to the agg_* structs instead? I haven't tried hacking
this, so maybe it's a silly idea.


4) lookup_hash_entries says

   /* check to see if we need to spill the tuple for this grouping set */

But that seems bogus, because AFAIK we can't spill tuples for grouping
sets. So maybe this should say just "grouping"?


5) Assert(nbuckets > 0);

I was curious what happens in case of extreme skew, when a lot/all rows
consistently falls into a single partition. So I did this:

   create table t (a int, b real);

   insert into t select i, random()
     from generate_series(-2000000000, 2000000000) s(i)
    where mod(hashint4(i), 16384) = 0;

   analyze t;

   set work_mem = '64kB';
   set max_parallel_workers_per_gather = 0;
   set enable_sort = 0;

   explain select a, sum(b) from t group by a;

                             QUERY PLAN                           
   ---------------------------------------------------------------
    HashAggregate  (cost=23864.26..31088.52 rows=244631 width=8)
      Group Key: a
      ->  Seq Scan on t  (cost=0.00..3529.31 rows=244631 width=8)
   (3 rows)

This however quickly fails on this assert in BuildTupleHashTableExt (see
backtrace1.txt):

   Assert(nbuckets > 0);

The value is computed in hash_choose_num_buckets, and there seem to be
no protections against returning bogus values like 0. So maybe we should
return

   Min(nbuckets, 1024)

or something like that, similarly to hash join. OTOH maybe it's simply
due to agg_refill_hash_table() passing bogus values to the function?


6) Another thing that occurred to me was what happens to grouping sets,
which we can't spill to disk. So I did this:

   create table t2 (a int, b int, c int);

   -- run repeatedly, until there are about 20M rows in t2 (1GB)
   with tx as (select array_agg(a) as a, array_agg(b) as b
                 from (select a, b from t order by random()) foo),
        ty as (select array_agg(a) AS a
                 from (select a from t order by random()) foo)
   insert into t2 select unnest(tx.a), unnest(ty.a), unnest(tx.b)
     from tx, ty;

   analyze t2;

This produces a table with two independent columns, skewed the same as
the column t.a. I don't know which of this actually matters, considering
grouping sets don't spill, so maybe the independence is sufficient and
the skew may be irrelevant?

And then do this:

   set work_mem = '200MB';
   set max_parallel_workers_per_gather = 0;
   set enable_sort = 0;

   explain select a, b, sum(c) from t2 group by cube (a,b);;

                                QUERY PLAN                              
   ---------------------------------------------------------------------
    MixedAggregate  (cost=0.00..833064.27 rows=2756495 width=16)
      Hash Key: a, b
      Hash Key: a
      Hash Key: b
      Group Key: ()
      ->  Seq Scan on t2  (cost=0.00..350484.44 rows=22750744 width=12)
   (6 rows)

which fails with segfault at execution time:

   tuplehash_start_iterate (tb=0x18, iter=iter@entry=0x2349340)
   870		for (i = 0; i < tb->size; i++)
   (gdb) bt
   #0  tuplehash_start_iterate (tb=0x18, iter=iter@entry=0x2349340)
   #1  0x0000000000654e49 in agg_retrieve_hash_table_in_memory ...

That's not surprising, because 0x18 pointer is obviously bogus. I guess
this is simply an offset 18B added to a NULL pointer?

Disabling hashagg spill (setting both GUCs to off) makes no difference,
but on master it fails like this:

   ERROR:  out of memory
   DETAIL:  Failed on request of size 3221225472 in memory context "ExecutorState".

which is annoying, but expected with an under-estimate and hashagg. And
much better than just crashing the whole cluster.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--hbghtqe47acw4lbv
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="backtrace1.txt"

#0  0x00007308e8e06e35 in raise () from /lib64/libc.so.6
#1  0x00007308e8df1895 in abort () from /lib64/libc.so.6
#2  0x00000000008c2aeb in ExceptionalCondition (conditionName=conditionName@entry=0x9f8583 "nbuckets > 0", errorType=errorType@entry=0x915029 "FailedAssertion", fileName=fileName@entry=0x9f84b7 "execGrouping.c", 
    lineNumber=lineNumber@entry=174) at assert.c:67
#3  0x000000000063f704 in BuildTupleHashTableExt (parent=parent@entry=0x2344538, inputDesc=<optimized out>, numCols=<optimized out>, keyColIdx=keyColIdx@entry=0x23614b0, eqfuncoids=0x2361528, hashfunctions=<optimized out>, 
    collations=0x232f130, nbuckets=0, additionalsize=16, metacxt=0x23461f0, tablecxt=0x23461f0, tempcxt=0x233e1b0, use_variable_hash_iv=false) at execGrouping.c:174
#4  0x00000000006534d1 in build_hash_table (aggstate=aggstate@entry=0x2344538, setno=<optimized out>, nbuckets=<optimized out>) at nodeAgg.c:1476
#5  0x000000000065504b in agg_refill_hash_table (aggstate=0x2344538) at nodeAgg.c:2474
#6  agg_retrieve_hash_table (aggstate=0x2344538) at nodeAgg.c:2594
#7  0x000000000065588d in agg_retrieve_direct (aggstate=<optimized out>) at nodeAgg.c:2141
#8  ExecAgg (pstate=<optimized out>) at nodeAgg.c:2022
#9  0x0000000000648579 in ExecProcNodeInstr (node=0x2344538) at execProcnode.c:460
#10 0x000000000064116b in ExecProcNode (node=0x2344538) at ../../../src/include/executor/executor.h:245
#11 ExecutePlan (execute_once=<optimized out>, dest=0xa39c00 <donothingDR>, direction=<optimized out>, numberTuples=0, sendTuples=<optimized out>, operation=CMD_SELECT, use_parallel_mode=<optimized out>, planstate=0x2344538, 
    estate=0x2344300) at execMain.c:1646
#12 standard_ExecutorRun (queryDesc=0x23308a0, direction=<optimized out>, count=0, execute_once=<optimized out>) at execMain.c:364
#13 0x00000000005daea9 in ExplainOnePlan (plannedstmt=plannedstmt@entry=0x2330788, into=into@entry=0x0, es=es@entry=0x232dd40, queryString=queryString@entry=0x22617b0 "explain analyze select a, b, sum(c) from t2 group by 1, 2;", 
    params=params@entry=0x0, queryEnv=queryEnv@entry=0x0, planduration=0x7ffdc444edc0) at explain.c:549
#14 0x00000000005db1d5 in ExplainOneQuery (queryEnv=0x0, params=0x0, queryString=0x22617b0 "explain analyze select a, b, sum(c) from t2 group by 1, 2;", es=0x232dd40, into=0x0, cursorOptions=<optimized out>, query=<optimized out>)
    at explain.c:385
#15 ExplainOneQuery (query=<optimized out>, cursorOptions=<optimized out>, into=0x0, es=0x232dd40, queryString=0x22617b0 "explain analyze select a, b, sum(c) from t2 group by 1, 2;", params=0x0, queryEnv=0x0) at explain.c:353
#16 0x00000000005db79e in ExplainQuery (pstate=pstate@entry=0x22cb5d0, stmt=stmt@entry=0x22629b0, params=params@entry=0x0, dest=dest@entry=0x22cb538) at ../../../src/include/nodes/nodes.h:590
#17 0x00000000007b47d3 in standard_ProcessUtility (pstmt=0x2262b28, queryString=0x22617b0 "explain analyze select a, b, sum(c) from t2 group by 1, 2;", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x22cb538, 
    completionTag=0x7ffdc444ef60 "") at utility.c:836
#18 0x00000000007b1986 in PortalRunUtility (portal=0x22c73e0, pstmt=0x2262b28, isTopLevel=<optimized out>, setHoldSnapshot=<optimized out>, dest=0x22cb538, completionTag=0x7ffdc444ef60 "") at pquery.c:1175
#19 0x00000000007b28b0 in FillPortalStore (portal=0x22c73e0, isTopLevel=<optimized out>) at ../../../src/include/nodes/nodes.h:590
#20 0x00000000007b3405 in PortalRun (portal=portal@entry=0x22c73e0, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true, dest=dest@entry=0x2348938, altdest=altdest@entry=0x2348938, 
    completionTag=0x7ffdc444f190 "") at pquery.c:765
#21 0x00000000007aee40 in exec_simple_query (query_string=0x22617b0 "explain analyze select a, b, sum(c) from t2 group by 1, 2;") at postgres.c:1227
#22 0x00000000007b0903 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x228f3d8, dbname=<optimized out>, username=<optimized out>) at postgres.c:4291
#23 0x000000000072f51e in BackendRun (port=0x2287140, port=0x2287140) at postmaster.c:4498
#24 BackendStartup (port=0x2287140) at postmaster.c:4189
#25 ServerLoop () at postmaster.c:1727
#26 0x0000000000730317 in PostmasterMain (argc=argc@entry=3, argv=argv@entry=0x225b390) at postmaster.c:1400
#27 0x000000000047e191 in main (argc=3, argv=0x225b390) at main.c:210

--hbghtqe47acw4lbv
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="backtrace2.txt"

tuplehash_start_iterate (tb=0x18, iter=iter@entry=0x2349340) at ../../../src/include/lib/simplehash.h:870
870		for (i = 0; i < tb->size; i++)
(gdb) bt
#0  tuplehash_start_iterate (tb=0x18, iter=iter@entry=0x2349340) at ../../../src/include/lib/simplehash.h:870
#1  0x0000000000654e49 in agg_retrieve_hash_table_in_memory (aggstate=0x23479e8) at nodeAgg.c:2664
#2  agg_retrieve_hash_table (aggstate=0x23479e8) at nodeAgg.c:2591
#3  0x000000000065588d in agg_retrieve_direct (aggstate=<optimized out>) at nodeAgg.c:2141
#4  ExecAgg (pstate=<optimized out>) at nodeAgg.c:2022
#5  0x0000000000648579 in ExecProcNodeInstr (node=0x23479e8) at execProcnode.c:460
#6  0x000000000064116b in ExecProcNode (node=0x23479e8) at ../../../src/include/executor/executor.h:245
#7  ExecutePlan (execute_once=<optimized out>, dest=0xa39c00 <donothingDR>, direction=<optimized out>, numberTuples=0, sendTuples=<optimized out>, operation=CMD_SELECT, use_parallel_mode=<optimized out>, planstate=0x23479e8, 
    estate=0x23477b0) at execMain.c:1646
#8  standard_ExecutorRun (queryDesc=0x2345d68, direction=<optimized out>, count=0, execute_once=<optimized out>) at execMain.c:364
#9  0x00000000005daea9 in ExplainOnePlan (plannedstmt=plannedstmt@entry=0x2345c50, into=into@entry=0x0, es=es@entry=0x2285690, queryString=queryString@entry=0x22617b0 "explain analyze select a, b, sum(c) from t2 group by cube (a,b);", 
    params=params@entry=0x0, queryEnv=queryEnv@entry=0x0, planduration=0x7ffdc444edc0) at explain.c:549
#10 0x00000000005db1d5 in ExplainOneQuery (queryEnv=0x0, params=0x0, queryString=0x22617b0 "explain analyze select a, b, sum(c) from t2 group by cube (a,b);", es=0x2285690, into=0x0, cursorOptions=<optimized out>, query=<optimized out>)
    at explain.c:385
#11 ExplainOneQuery (query=<optimized out>, cursorOptions=<optimized out>, into=0x0, es=0x2285690, queryString=0x22617b0 "explain analyze select a, b, sum(c) from t2 group by cube (a,b);", params=0x0, queryEnv=0x0) at explain.c:353
#12 0x00000000005db79e in ExplainQuery (pstate=pstate@entry=0x2283700, stmt=stmt@entry=0x2262bc0, params=params@entry=0x0, dest=dest@entry=0x22855f8) at ../../../src/include/nodes/nodes.h:590
#13 0x00000000007b47d3 in standard_ProcessUtility (pstmt=0x2262d38, queryString=0x22617b0 "explain analyze select a, b, sum(c) from t2 group by cube (a,b);", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x22855f8, 
    completionTag=0x7ffdc444ef60 "") at utility.c:836
#14 0x00000000007b1986 in PortalRunUtility (portal=0x22cf3f0, pstmt=0x2262d38, isTopLevel=<optimized out>, setHoldSnapshot=<optimized out>, dest=0x22855f8, completionTag=0x7ffdc444ef60 "") at pquery.c:1175
#15 0x00000000007b28b0 in FillPortalStore (portal=0x22cf3f0, isTopLevel=<optimized out>) at ../../../src/include/nodes/nodes.h:590
#16 0x00000000007b3405 in PortalRun (portal=portal@entry=0x22cf3f0, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true, dest=dest@entry=0x232ef00, altdest=altdest@entry=0x232ef00, 
    completionTag=0x7ffdc444f190 "") at pquery.c:765
#17 0x00000000007aee40 in exec_simple_query (query_string=0x22617b0 "explain analyze select a, b, sum(c) from t2 group by cube (a,b);") at postgres.c:1227
#18 0x00000000007b0903 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x228f4b8, dbname=<optimized out>, username=<optimized out>) at postgres.c:4291
#19 0x000000000072f51e in BackendRun (port=0x2287140, port=0x2287140) at postmaster.c:4498
#20 BackendStartup (port=0x2287140) at postmaster.c:4189
#21 ServerLoop () at postmaster.c:1727
#22 0x0000000000730317 in PostmasterMain (argc=argc@entry=3, argv=argv@entry=0x225b390) at postmaster.c:1400
#23 0x000000000047e191 in main (argc=3, argv=0x225b390) at main.c:210

--hbghtqe47acw4lbv--