Re: ExplainProperty* and units

From: Andres Freund <andres(at)anarazel(dot)de>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: ExplainProperty* and units
Date: 2018-03-14 16:48:32
Message-ID: 20180314164832.n56wt7zcbpzi6zxe@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On 2018-03-13 17:27:40 -0700, Andres Freund wrote:
> Hi,
>
> while adding EXPLAIN support for JITing (displaying time spent etc), I
> got annoyed by the amount of duplication required. There's a fair amount
> of
> if (es->format == EXPLAIN_FORMAT_TEXT)
> appendStringInfo(es->str, "Execution time: %.3f ms\n",
> 1000.0 * totaltime);
> else
> ExplainPropertyFloat("Execution Time", 1000.0 * totaltime,
> which is fairly redundant.
>
> In the attached *POC* patch I've added a 'unit' parameter to the numeric
> ExplainProperty* functions, which EXPLAIN_FORMAT_TEXT adds to the
> output. This can avoid the above and other similar branches (of which
> the JIT patch would add a number).

If we do this, and I think we should, I'm inclined to also commit a
patch that renames

/*
* Explain a long-integer-valued property.
*/
void
ExplainPropertyLong(const char *qlabel, const char *unit, long value,
ExplainState *es)
{
char buf[32];

snprintf(buf, sizeof(buf), "%ld", value);
ExplainProperty(qlabel, unit, buf, true, es);
}

and changes its argument type. Because passing long is just plain
unhelpful for 32bit platforms and windows. We should just always use
64bits here. Only thing I wonder is if we shouldn't just *remove*
ExplainPropertyLong and make ExplainPropertyInteger accept 64bits of
input - the effort of passing and printing a 64bit integer will never be
relevant for explain.

- Andres

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-03-14 17:32:10 Re: ExplainProperty* and units
Previous Message Stephen Frost 2018-03-14 16:41:17 Re: Comment fixes in create_grouping_paths() add_paths_to_append_rel()