public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ibrar Ahmed <[email protected]>
To: Tom Lane <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Peter Geoghegan <[email protected]>
Cc: Greg Stark <[email protected]>
Cc: vignesh C <[email protected]>
Cc: David G. Johnston <[email protected]>
Cc: [email protected]
Subject: Re: explain analyze rows=%.0f
Date: Wed, 4 Jan 2023 20:04:53 +0500
Message-ID: <CALtqXTfM3GJbFsB63ajfHcYvFhUA0ySX4f4MVS3Dau2vopU+Rw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CA+TgmoZv8DvmeQHQAX+rkn68id2N+DoUhFZB36d-_w1tyAeqeA@mail.gmail.com>
<[email protected]>
<CAA4eK1Lmo0FX6Nfr_k3-UX1Hpei1H-xPHL6Wwx7SLUP3s+O1mA@mail.gmail.com>
<CA+Tgmob0BBeK6GEMUQBPiv0kJpTJSEgX+OUD2FeY42zWVgJuLw@mail.gmail.com>
<[email protected]>
On Sun, Nov 6, 2022 at 10:12 AM Tom Lane <[email protected]> wrote:
> Robert Haas <[email protected]> writes:
> > On Fri, Jul 22, 2022 at 6:47 AM Amit Kapila <[email protected]>
> 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
view thread (3+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: explain analyze rows=%.0f
In-Reply-To: <CALtqXTfM3GJbFsB63ajfHcYvFhUA0ySX4f4MVS3Dau2vopU+Rw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox