Date: Fri, 27 Mar 2020 02:31:08 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Richard Guo <guofenglinux@gmail.com>
Cc: Jeff Davis <pgsql@j-davis.com>, Justin Pryzby <pryzby@telsasoft.com>,
	Andres Freund <andres@anarazel.de>,
	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>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Memory-Bounded Hash Aggregation
Message-ID: <20200327013108.tiskardwkqr5otg6@development>
References: <9759ab2d0c2c28a175e11efe1788d37dc7e1b768.camel@j-davis.com>
 <20200222180020.ej5gvfcaf2semgji@alap3.anarazel.de>
 <5c371df7cee903e8cd4c685f90c6c72086d3a2dc.camel@j-davis.com>
 <20200224232928.45bcjmdxgnobijgi@alap3.anarazel.de>
 <6e7c269b9a84ff75fefcad8ab2d4758f03581e98.camel@j-davis.com>
 <bd6edb713f0e3c9ecb7f08a6b4bb3f469c6cf152.camel@j-davis.com>
 <20200312210146.GG29065@telsasoft.com>
 <b2af57f4f8a27b34a0efb80ce823f5cd2bbb9164.camel@j-davis.com>
 <2688fb94f3d0e571768bf851b7fde0eae780e898.camel@j-davis.com>
 <CAMbWs4_W8fYbAn8KxgidAaZHON_Oo08OYn9ze=7remJymLqo5g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAMbWs4_W8fYbAn8KxgidAaZHON_Oo08OYn9ze=7remJymLqo5g@mail.gmail.com>
Precedence: bulk

On Thu, Mar 26, 2020 at 05:56:56PM +0800, Richard Guo wrote:
>Hello,
>
>When calculating the disk costs of hash aggregation that spills to disk,
>there is something wrong with how we determine depth:
>
>>            depth = ceil( log(nbatches - 1) / log(num_partitions) );
>
>If nbatches is some number between 1.0 and 2.0, we would have a negative
>depth. As a result, we may have a negative cost for hash aggregation
>plan node, as described in [1].
>
>I don't think 'log(nbatches - 1)' is what we want here. Should it be
>just '(nbatches - 1)'?
>

I think using log() is correct, but why should we allow fractional
nbatches values between 1.0 and 2.0? You either have 1 batch or 2
batches, you can't have 1.5 batches. So I think the issue is here

   nbatches = Max((numGroups * hashentrysize) / mem_limit,
                  numGroups / ngroups_limit );

and we should probably do

   nbatches = ceil(nbatches);

right after it.


regards

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