Date: Fri, 26 Jun 2020 01:23:16 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Bruce Momjian <bruce@momjian.us>,
	David Rowley <dgrowleyml@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	Melanie Plageman <melanieplageman@gmail.com>,
	pgsql-hackers@postgresql.org
Subject: Re: Default setting for enable_hashagg_disk
Message-ID: <20200625232316.cevmhw7cgra6bu6e@development>
References: 
 <CA+TgmoY4-zSMFf8XbpO6uRsMX4vNPPLXvtajAZpG3f9eoEyjdA@mail.gmail.com>
 <129c8f1739957589584d9525e1d8782eae3f412f.camel@j-davis.com>
 <CAApHDvqFZikXhAGW=UKZKq1_FzHy+XzmUzAJiNj6RWyTHH4UfA@mail.gmail.com>
 <20200624090628.GA19640@momjian.us>
 <20200624123843.GX4107@telsasoft.com>
 <2214502.1593019796@sss.pgh.pa.us>
 <20200624183203.cnqcol27ruastit7@development>
 <2217145.1593024050@sss.pgh.pa.us>
 <20200624193122.cqolrvl44fh4w4s4@alap3.anarazel.de>
 <3e07d41d5104466cb46e3fd076c47989fe65f085.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <3e07d41d5104466cb46e3fd076c47989fe65f085.camel@j-davis.com>
Precedence: bulk

On Thu, Jun 25, 2020 at 09:42:33AM -0700, Jeff Davis wrote:
>On Wed, 2020-06-24 at 12:31 -0700, Andres Freund wrote:
>> nodeAgg.c already treats those separately:
>>
>> void
>> hash_agg_set_limits(double hashentrysize, uint64 input_groups, int
>> used_bits,
>> 					Size *mem_limit, uint64
>> *ngroups_limit,
>> 					int *num_partitions)
>> {
>> 	int			npartitions;
>> 	Size		partition_mem;
>>
>> 	/* if not expected to spill, use all of work_mem */
>> 	if (input_groups * hashentrysize < work_mem * 1024L)
>> 	{
>> 		if (num_partitions != NULL)
>> 			*num_partitions = 0;
>> 		*mem_limit = work_mem * 1024L;
>> 		*ngroups_limit = *mem_limit / hashentrysize;
>> 		return;
>> 	}
>
>The reason this code exists is to decide how much of work_mem to set
>aside for spilling (each spill partition needs an IO buffer).
>
>The alternative would be to fix the number of partitions before
>processing a batch, which didn't seem ideal. Or, we could just ignore
>the memory required for IO buffers, like HashJoin.
>

I think the conclusion from the recent HashJoin discussions is that not
accounting for BufFiles is an issue, and we want to fix it. So repeating
that for HashAgg would be a mistake, IMHO.

>Granted, this is an example where an underestimate can give an
>advantage, but I don't think we want to extend the concept into other
>areas.
>

I agree.


regards

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