pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
From: Justin Pryzby <pryzby@telsasoft.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Melanie Plageman <melanieplageman@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Taylor Vesely <tvesely@pivotal.io>
Cc: Adam Lee <ali@pivotal.io>
Cc: Melanie Plageman <mplageman@pivotal.io>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Memory-Bounded Hash Aggregation
Date: Thu, 12 Mar 2020 16:01:46 -0500
Message-ID: <20200312210146.GG29065@telsasoft.com> (raw)
In-Reply-To: <bd6edb713f0e3c9ecb7f08a6b4bb3f469c6cf152.camel@j-davis.com>
References: <20200218185749.srk733n4z57lzxf2@development>
	<7612ea4d636b6341d0606f4e852d584798e04901.camel@j-davis.com>
	<20200219191636.gvdywx32kwbix6kd@development>
	<20200221202212.ssb2qpmdgrnx52sj@alap3.anarazel.de>
	<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>

On Wed, Mar 11, 2020 at 11:55:35PM -0700, Jeff Davis wrote:
>  * tweaked EXPLAIN output some more
> Unless I (or someone else) finds something significant, this is close
> to commit.

Thanks for working on this ; I finally made a pass over the patch.

+++ b/doc/src/sgml/config.sgml
+      <term><varname>enable_groupingsets_hash_disk</varname> (<type>boolean</type>)
+        Enables or disables the query planner's use of hashed aggregation for
+        grouping sets when the size of the hash tables is expected to exceed
+        <varname>work_mem</varname>.  See <xref
+        linkend="queries-grouping-sets"/>.  Note that this setting only
+        affects the chosen plan; execution time may still require using
+        disk-based hash aggregation.  ...
...
+      <term><varname>enable_hashagg_disk</varname> (<type>boolean</type>)
+        ... This only affects the planner choice;
+        execution time may still require using disk-based hash
+        aggregation. The default is <literal>on</literal>.

I don't understand what's meant by "the chosen plan".
Should it say, "at execution ..." instead of "execution time" ?

+        Enables or disables the query planner's use of hashed aggregation plan
+        types when the memory usage is expected to exceed

Either remove "plan types" for consistency with enable_groupingsets_hash_disk,
Or add it there.  Maybe it should say "when the memory usage would OTHERWISE BE
expected to exceed.."

+show_hashagg_info(AggState *aggstate, ExplainState *es)
+{
+	Agg		*agg	   = (Agg *)aggstate->ss.ps.plan;
+	long	 memPeakKb = (aggstate->hash_mem_peak + 1023) / 1024;

I see this partially duplicates my patch [0] to show memory stats for (at
Andres' suggestion) all of execGrouping.c.  Perhaps you'd consider naming the
function something more generic in case my patch progresses ?  I'm using:
|show_tuplehash_info(HashTableInstrumentation *inst, ExplainState *es);

Mine also shows:
|ExplainPropertyInteger("Original Hash Buckets", NULL,
|ExplainPropertyInteger("Peak Memory Usage (hashtable)", "kB",
|ExplainPropertyInteger("Peak Memory Usage (tuples)", "kB",

[0] https://www.postgresql.org/message-id/20200306213310.GM684%40telsasoft.com

You added hash_mem_peak and hash_batches_used to struct AggState.
In my 0001 patch, I added instrumentation to struct TupleHashTable, and in my
0005 patch I move it into AggStatePerHashData and other State nodes.

+	if (from_tape)
+		partition_mem += HASHAGG_READ_BUFFER_SIZE;
+	partition_mem = npartitions * HASHAGG_WRITE_BUFFER_SIZE;

=> That looks wrong ; should say += ?

+			gettext_noop("Enables the planner's use of hashed aggregation plans that are expected to exceed work_mem."),

should say:
"when the memory usage is otherwise be expected to exceed.."

-- 
Justin





view thread (81+ messages)  latest in thread

Message-ID: <20200312210146.GG29065@telsasoft.com>
Permalink:  ../20200312210146.GG29065@telsasoft.com/
Also on:    postgresql.org/message-id/20200312210146.GG29065@telsasoft.com

 · 

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: pryzby@telsasoft.com, pgsql@j-davis.com, andres@anarazel.de, tomas.vondra@2ndquadrant.com, melanieplageman@gmail.com, hlinnaka@iki.fi, tvesely@pivotal.io, ali@pivotal.io, mplageman@pivotal.io
  Subject: Re: Memory-Bounded Hash Aggregation
  In-Reply-To: <20200312210146.GG29065@telsasoft.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox