Horton

Thread

Commits

  1. Add 'unit' parameter to ExplainProperty{Integer,Float}.

  2. Make ExplainPropertyInteger accept 64bit input, remove *Long variant.

  1. ExplainProperty* and units

    Andres Freund <andres@anarazel.de> — 2018-03-14T00:27:40Z

    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).

The most valid counterargument I see is that in many cases, particularly
inside plans, we have more specific output for text mode anyway. Which
means there we'll not benefit much.  But I think that's a) considerably
done due to backward compatibility concerns b) verbosity concerns inside
plans, which obviously can be complicated.  Therefore I think it's
perfectly reasonable to avoid specific branches for data that's only
going to be displayed once per plan?

We also could add separate ExplainProperty*Unit(...) functions, but I
don't really see a need.

Comments?

Greetings,

Andres Freund

  2. Re: ExplainProperty* and units

    David Rowley <david.rowley@2ndquadrant.com> — 2018-03-14T01:34:38Z

    On 14 March 2018 at 13:27, Andres Freund <andres@anarazel.de> wrote:
> 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).

> Comments?

This seems like a good change. It would be nice to see us completely
get rid of all the appendStringInfo* calls, apart from in the
functions which meant to handle the behaviour specific to the explain
format. This is a step towards that, so has my vote.

Functions like show_hash_info are making it difficult to do more in this area.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  3. Re: ExplainProperty* and units

    Andres Freund <andres@anarazel.de> — 2018-03-14T16:07:09Z

    Hi,

On 2018-03-14 14:34:38 +1300, David Rowley wrote:
> On 14 March 2018 at 13:27, Andres Freund <andres@anarazel.de> wrote:
> > 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).
> 
> > Comments?
> 
> This seems like a good change.

Cool.


> It would be nice to see us completely get rid of all the
> appendStringInfo* calls, apart from in the functions which meant to
> handle the behaviour specific to the explain format. This is a step
> towards that, so has my vote.

Do you mean you want to only use ExplainProperty*? Given the desire of
density for plan output that seem hard to achieve.


> Functions like show_hash_info are making it difficult to do more in this area.

Hm, what's the problem?

Greetings,

Andres Freund

  4. Re: ExplainProperty* and units

    Andres Freund <andres@anarazel.de> — 2018-03-14T16:48:32Z

    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

  5. Re: ExplainProperty* and units

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-14T17:32:10Z

    Andres Freund <andres@anarazel.de> writes:
> If we do this, and I think we should, I'm inclined to also commit a
> patch that renames ExplainPropertyLong
> and changes its argument type. Because passing long is just plain
> unhelpful for 32bit platforms and windows.  We should just always use
> 64bits here.

+1 --- I'm pretty certain that function predates our requirement that
all platforms support int64.

> 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.

-0.5 ... everywhere else, we mean "int32" when we say "int", and I don't
think it's worth the potential confusion to do it differently here.

			regards, tom lane

  6. Re: ExplainProperty* and units

    Andres Freund <andres@anarazel.de> — 2018-03-14T17:35:47Z

    Hi,

On 2018-03-14 13:32:10 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > If we do this, and I think we should, I'm inclined to also commit a
> > patch that renames ExplainPropertyLong
> > and changes its argument type. Because passing long is just plain
> > unhelpful for 32bit platforms and windows.  We should just always use
> > 64bits here.
> 
> +1 --- I'm pretty certain that function predates our requirement that
> all platforms support int64.

Cool.


> > 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.
> 
> -0.5 ... everywhere else, we mean "int32" when we say "int", and I don't
> think it's worth the potential confusion to do it differently here.

So ExplainPropertyInt{32,64}? I agree that it's not what we do
elsewhere, but I just don't see any use in actually having two
functions. I think one counter argument to your point is that it's not
an 'int' named function, it's Integer which should fit both 32 and 64
bit ones ;)

Greetings,

Andres Freund

  7. Re: ExplainProperty* and units

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-14T17:39:52Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-03-14 13:32:10 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> 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.

>> -0.5 ... everywhere else, we mean "int32" when we say "int", and I don't
>> think it's worth the potential confusion to do it differently here.

> So ExplainPropertyInt{32,64}?

I was thinking more of replacing ExplainPropertyLong with
ExplainPropertyInt64 and leaving ExplainPropertyInteger as-is.
Don't see the value in renaming that one just to rename it.

			regards, tom lane

  8. Re: ExplainProperty* and units

    Andres Freund <andres@anarazel.de> — 2018-03-14T17:58:53Z

    Hi,

On 2018-03-13 17:27:40 -0700, Andres Freund wrote:
> 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).

Here's a series of two, a bit more carefully done, patches.  First
removes ExplainPropertyLong, expands ExplainPropertyInteger to
64bits. Second adds the unit argument.

Whether we want to go for the first one, or revise first patch to just
rename ExplainPropertyInt64 remains to be discussed.  I still slightly
prefer the approach presented here.

- Andres

  9. Re: ExplainProperty* and units

    David Rowley <david.rowley@2ndquadrant.com> — 2018-03-15T01:52:32Z

    On 15 March 2018 at 05:07, Andres Freund <andres@anarazel.de> wrote:
> On 2018-03-14 14:34:38 +1300, David Rowley wrote:
>> It would be nice to see us completely get rid of all the
>> appendStringInfo* calls, apart from in the functions which meant to
>> handle the behaviour specific to the explain format. This is a step
>> towards that, so has my vote.
>
> Do you mean you want to only use ExplainProperty*? Given the desire of
> density for plan output that seem hard to achieve.

You're probably right, there are many cases where we want some
specific compact format for text, but care less about the verbosity in
JSON/XML.

Probably there is room to improve this a little by having something
like ExplainPropertyFormat. But not for today... I only mentioned it
as I saw your patch as a step in the right direction.

>> Functions like show_hash_info are making it difficult to do more in this area.
>
> Hm, what's the problem?

Basically the special cases for TEXT types plus manually building
strings with appendStringInfo and manually dealing with indentation.
An ExplainPropertyFormat could help some of that, but wouldn't get rid
of the special TEXT case.


-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  10. Re: ExplainProperty* and units

    Andres Freund <andres@anarazel.de> — 2018-03-16T21:25:12Z

    On 2018-03-14 13:32:10 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > 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.
> 
> -0.5 ... everywhere else, we mean "int32" when we say "int", and I don't
> think it's worth the potential confusion to do it differently here.

Since I'm +1 on this (I don't think Integer is the same as int, there's
no benefit in having two functions, and I've already written the patch
this way), I'm inclined to go with one function.  Does anybody else
have an opinion?

- Andres