On Sun, Nov 6, 2022 at 10:12 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <robertmh...@gmail.com> writes: > > On Fri, Jul 22, 2022 at 6:47 AM Amit Kapila <amit.kapil...@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
