Re: explain HashAggregate to report bucket and memory stats

Hi,

On 2020-03-01 09:45:40 -0600, Justin Pryzby wrote:
> +/*
> + * Show hash bucket stats and (optionally) memory.
> + */
> +static void
> +show_tuplehash_info(HashTableInstrumentation *inst, ExplainState *es)
> +{
> + long spacePeakKb_tuples = (inst->space_peak_tuples + 1023) / 1024,
> + spacePeakKb_hash = (inst->space_peak_hash + 1023) / 1024;

Let's not add further uses of long. It's terrible because it has
different widths on 64bit windows and everything else. Specify the
widths explicitly, or use something like size_t.

> + if (es->format != EXPLAIN_FORMAT_TEXT)
> + {
> + ExplainPropertyInteger("Hash Buckets", NULL,
> + inst->nbuckets, es);
> + ExplainPropertyInteger("Original Hash Buckets", NULL,
> + inst->nbuckets_original, es);
> + ExplainPropertyInteger("Peak Memory Usage (hashtable)", "kB",
> + spacePeakKb_hash, es);
> + ExplainPropertyInteger("Peak Memory Usage (tuples)", "kB",
> + spacePeakKb_tuples, es);

And then you're passing the long to ExplainPropertyInteger which accepts
a int64, making the use of long above particularly suspicious.

I wonder if it would make sense to add a ExplainPropertyBytes(), that
would do the rounding etc automatically. It could then also switch units
as appropriate.

> + }
> + else if (!inst->nbuckets)
> + ; /* Do nothing */
> + else
> + {
> + if (inst->nbuckets_original != inst->nbuckets)
> + {
> + ExplainIndentText(es);
> + appendStringInfo(es->str,
> + "Buckets: %ld (originally %ld)",
> + inst->nbuckets,
> + inst->nbuckets_original);
> + }
> + else
> + {
> + ExplainIndentText(es);
> + appendStringInfo(es->str,
> + "Buckets: %ld",
> + inst->nbuckets);
> + }
> +
> + if (es->analyze)
> + appendStringInfo(es->str,
> + " Memory Usage: hashtable: %ldkB, tuples: %ldkB",
> + spacePeakKb_hash, spacePeakKb_tuples);
> + appendStringInfoChar(es->str, '\n');

I'm not sure I like the alternative output formats here. All the other
fields are separated with a comma, but the original size is in
parens. I'd probably just format it as "Buckets: %lld " and then add
", Original Buckets: %lld" when differing.

Also, %ld is problematic, because it's only 32bit wide on some platforms
(including 64bit windows), but 64bit on others. The easiest way to deal
with that is to use %lld and cast the argument to long long - yes that's
a sad workaround.

> +/* Update instrumentation stats */
> +void
> +UpdateTupleHashTableStats(TupleHashTable hashtable, bool initial)
> +{
> + hashtable->instrument.nbuckets = hashtable->hashtab->size;
> + if (initial)
> + {
> + hashtable->instrument.nbuckets_original = hashtable->hashtab->size;
> + hashtable->instrument.space_peak_hash = hashtable->hashtab->size *
> + sizeof(TupleHashEntryData);
> + hashtable->instrument.space_peak_tuples = 0;
> + }
> + else
> + {
> +#define maxself(a,b) a=Max(a,b)
> + /* hashtable->entrysize includes additionalsize */
> + maxself(hashtable->instrument.space_peak_hash,
> + hashtable->hashtab->size * sizeof(TupleHashEntryData));
> + maxself(hashtable->instrument.space_peak_tuples,
> + hashtable->hashtab->members * hashtable->entrysize);
> +#undef maxself
> + }
> +}

Not a fan of this macro.

I'm also not sure I understand what you're trying to do here?

> diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
> index f457b5b150..b173b32cab 100644
> --- a/src/test/regress/expected/aggregates.out
> +++ b/src/test/regress/expected/aggregates.out
> @@ -517,10 +517,11 @@ order by 1, 2;
> -> HashAggregate
> Output: s2.s2, sum((s1.s1 + s2.s2))
> Group Key: s2.s2
> + Buckets: 4
> -> Function Scan on pg_catalog.generate_series s2
> Output: s2.s2
> Function Call: generate_series(1, 3)
> -(14 rows)
> +(15 rows)

These tests probably won't be portable. The number of hash buckets
calculated will e.g. depend onthe size of the contained elements. And
that'll e.g. will depend on whether pointers are 4 or 8 bytes.

Greetings,

Andres Freund