Re: explain analyze rows=%.0f

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

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