Re: explain analyze rows=%.0f

On Sun, Nov 6, 2022 at 10:12 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Fri, Jul 22, 2022 at 6:47 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>
>
Thanks, I have modified everything as suggested, except one point

> 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.
>
> In that, we need to adjust a lot of test case outputs.

* We need some doc adjustments, surely, to explain what the heck this
> means.
>

> regards, tom lane
>

--
Ibrar Ahmed