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