Date: Thu, 2 Jul 2020 21:46:49 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Peter Geoghegan <pg@bowt.ie>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	Bruce Momjian <bruce@momjian.us>,
	Robert Haas <robertmhaas@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	David Rowley <dgrowleyml@gmail.com>, Jeff Davis <pgsql@j-davis.com>,
	Melanie Plageman <melanieplageman@gmail.com>,
	pgsql-hackers@postgresql.org
Subject: Re: Default setting for enable_hashagg_disk (hash_mem)
Message-ID: <20200703024649.GJ4107@telsasoft.com>
References: <20200626230020.GD3337@momjian.us>
 <20200626235850.gvl3lpfyeobu4evi@development>
 <CAH2-Wz=osB4oi_nH8MnosYhVVSNOm5q3=exGe-b3q6gWOgf98w@mail.gmail.com>
 <20200627104141.gq7d3hm2tvoqgjjs@development>
 <CAH2-Wzn6M7FwwgXe-mSnmCuMuF9OnFZW=m0X6JMOEYt4VfJTgQ@mail.gmail.com>
 <20200629150655.ethstxkf3bkqoown@development>
 <CAH2-WzmnaKVmZFNzcwAyGzvetLTO_aHkVVptMaBQ-gv1OjFh2g@mail.gmail.com>
 <20200629212229.n3afgzq6xpxrr4cu@development>
 <CAH2-Wzmt+b6=TQmavHKbsiD9g4R+iFaMEK89P88Bq5w=vgdnTQ@mail.gmail.com>
 <CAH2-WzmD+i1pG6rc1+Cjc4V6EaFJ_qSuKCCHVnH=oruqD-zqow@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAH2-WzmD+i1pG6rc1+Cjc4V6EaFJ_qSuKCCHVnH=oruqD-zqow@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Jul 02, 2020 at 07:05:48PM -0700, Peter Geoghegan wrote:
> On Mon, Jun 29, 2020 at 2:46 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > On Mon, Jun 29, 2020 at 2:22 PM Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
> > > I agree with this, and I'm mostly OK with having hash_mem. In fact, from
> > > the proposals in this thread I like it the most - as long as it's used
> > > both during planning and execution. It's a pretty clear solution.
> >
> > Great.
> >
> > It's not trivial to write the patch, since there are a few tricky
> > cases. For example, maybe there is some subtlety in nodeAgg.c with
> > AGG_MIXED cases.
> 
> Attached is an attempt at this. I have not been particularly thorough,

Thanks for putting it together, I agree that hash_mem seems to be an obvious
"escape hatch" that generalizes existing GUCs and independently useful.

> anything else). I still think that the new GUC should work as a
> multiplier of work_mem, or something else along those lines, though
> for now it's just an independent work_mem used for hashing. I bring it
> up again because I'm concerned about users that upgrade to Postgres 13
> incautiously, and find that hashing uses *less* memory than before.
> Many users probably get away with setting work_mem quite high across
> the board. At the very least, hash_mem should be ignored when it's set
> to below work_mem (which isn't what the patch does).

I feel it should same as work_mem, as it's written, and not a multiplier.

And actually I don't think a lower value should be ignored: "mechanism not
policy".  Do we refuse atypical values of maintenance_work_mem < work_mem ?

I assumed that hash_mem would default to -1, which would mean "fall back to
work_mem".  We'd then advise users to increase it if they have an issue in v13
with performance of hashes spilled to disk.  (And maybe in other cases, too.)

I read the argument that hash tables are a better use of RAM than sort.
However it seems like setting the default to greater than work_mem is a
separate change than providing the mechanism allowing user to do so.  I guess
the change in default is intended to mitigate the worst possible behavior
change someone might experience in v13 hashing, and might be expected to
improve "out of the box" performance.  I'm not opposed to it, but it's not an
essential part of the patch.

In nodeHash.c, you missed an underscore:
+        * Target in-memory hashtable size is hashmem kilobytes.

-- 
Justin