Date: Sun, 23 Feb 2020 01:42:16 +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: <20200223004216.obvgyvmusnsscnsk@development>
References: 
 <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>
 <20200219191636.gvdywx32kwbix6kd@development>
 <732b17848c2333e50f146e656a9c4f75ff818f05.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <732b17848c2333e50f146e656a9c4f75ff818f05.camel@j-davis.com>
Precedence: bulk

On Thu, Feb 20, 2020 at 04:56:38PM -0800, Jeff Davis wrote:
>On Wed, 2020-02-19 at 20:16 +0100, Tomas Vondra wrote:
>> 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.
>
>Looks like you meant to include some example explain output, but I
>think I understand what you mean. I'll look into it.
>

Oh, right. What I wanted to include is this code snippet:

   if (es->analyze)
       show_hashagg_info((AggState *) planstate, es);

but I forgot to do the copy-paste.

>> 2) The ExecBuildAggTrans comment should probably explain "spilled".
>
>Done.
>
>> 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.
>
>There was a reason I didn't do it this way, but I'm trying to remember
>why. I'll look into this, also.
>
>> 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"?
>
>Yes, we can spill tuples for grouping sets. Unfortunately, I think my
>tests (which covered this case previously) don't seem to be exercising
>that path well now. I am going to improve my tests, too.
>
>> 5) Assert(nbuckets > 0);
>
>I did not repro this issue, but I did set a floor of 256 buckets.
>

Hmmm. I can reproduce it reliably (on the patch from 2020/02/18) but it
seems to only happen when the table is large enough. For me, doing

   insert into t select * from t;

until the table has ~7.8M rows does the trick. I can't reproduce it on
the current patch, so ensuring there are at least 256 buckets seems to
have helped. If I add an elog() to print nbuckets at the beginning of
hash_choose_num_buckets, I see it starts as 0 from time to time (and
then gets tweaked to 256).

I suppose this is due to how the input data is generated, i.e. all hash
values should fall to the first batch, so all other batches should be
empty. But in agg_refill_hash_table we use the number of input tuples as
a starting point for, which is how we get nbuckets = 0.

I think enforcing nbuckets to be at least 256 is OK.

>> which fails with segfault at execution time:
>
>Fixed. I was resetting the hash table context without setting the
>pointers to NULL.
>

Yep, can confirm it's no longer crashing for me.

>Thanks! I attached a new, rebased version. The fixes are quick fixes
>for now and I will revisit them after I improve my test cases (which
>might find more issues).
>

OK, sounds good.


regards

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