Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jul 22, 2022 at 6:47 AM Amit Kapila <amit.kapila16@gmail.com> wr=
ote:
>> I feel the discussion has slightly deviated which makes it unclear
>> whether this patch is required or not?
> My opinion is that showing some fractional digits at least when
> loops>1 would be better than what we have now. It might not be the
> best thing we could do, but it would be better than doing nothing.
Yeah, I think that is a reasonable compromise.
I took a brief look through the patch, and I have some review
comments:
* Code like this is pretty awful:
appendStringInfo(es->str,
(nloops =3D=3D 1 || !HAS_DECIMAL(rows)) ?
" rows=3D%.0f loops=3D%.0f)" : " rows=3D%=
.2f loops=3D%.0f)",
rows, nloops);
Don't use variable format strings. They're hard to read and they
probably defeat compile-time checks that the arguments match the
format string. You could use a "*" field width instead, ie
appendStringInfo(es->str,
" rows=3D%.*f loops=3D%.0f)",
(nloops =3D=3D 1 || !HAS_DECIMAL(rows)) ?=
2 : 0,
rows, nloops);
That'd also allow you to reduce the code churn you've added by
splitting some appendStringInfo calls.
* I'm fairly concerned about how stable this'll be in the buildfarm,
in particular I fear HAS_DECIMAL() is not likely to give consistent
results across platforms. I gather that an earlier version of the patch
tried to check whether the fractional part would be zero to two decimal
places, rather than whether it's exactly zero. Probably want to put
back something like that.
* Another thought is that the non-text formats tend to prize output
consistency over readability, so maybe we should just always use 2
fractional digits there, rather than trying to minimize visible changes.
* We need some doc adjustments, surely, to explain what the heck this
means.
regards, tom lane
