Thread: ExplainProperty* and units
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
Attachment
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
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
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
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
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
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
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
Attachment
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
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