Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pD5Kj-0000Kp-AR for pgsql-hackers@arkaria.postgresql.org; Wed, 04 Jan 2023 15:05:37 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1pD5Kh-0000gX-3V for pgsql-hackers@arkaria.postgresql.org; Wed, 04 Jan 2023 15:05:35 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pD5Kg-0000gO-PM for pgsql-hackers@lists.postgresql.org; Wed, 04 Jan 2023 15:05:34 +0000 Received: from mail-ot1-x331.google.com ([2607:f8b0:4864:20::331]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1pD5Ke-0003HH-Db for pgsql-hackers@lists.postgresql.org; Wed, 04 Jan 2023 15:05:34 +0000 Received: by mail-ot1-x331.google.com with SMTP id j16-20020a056830271000b0067202045ee9so20886207otu.7 for ; Wed, 04 Jan 2023 07:05:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=I4yKfWmXPA3auWbmbF8HBf0PUcDn/B7R7IjzQY5XmkI=; b=P92IMj/VxHuUYDui3jQ2ajzBTuTYXsNf+Kir6obbIkpYph+At4BpeDJVfcmuFLJ38t Uzk12oEZqdMHDrXMl/K8Atupou3b2K1MqIC+iLmqORcLmFv+vrdQJlVlSJHXqoFVRMo9 qDB+8Rd4R+jeqA/1Prr1syXmpmhFv0MdMx4ueVeXL79fbmf88syTFkrUXwl50fVPum0/ PkhrSPZd/w+CngLm4uIl96b7a79rGMztBSovOLRIIFhZLYnipTciZEa3nSUCgs/tiOdP K3KFvqZRUIG0ohg3YS8a9GsOlPQIx5TWRRZFUx3P83v7ASL4Ea9V11ofBySb0Ai5ZVgO /7GA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=I4yKfWmXPA3auWbmbF8HBf0PUcDn/B7R7IjzQY5XmkI=; b=l5cvS3R4GytCV5SDObAK0i4rw8u8TStZ8RH/3SJfEHGKcg61mAh4tUx66Hm4zMwd+9 qPCIF5mgZmghhqgIv/rQQlfqCjYhydlUAAADOinc+9fiwUYxLgHuhdnFcCSLtPzbmwRK IW2q4Bx3ewkYGcmlzrJNZZYuvqdDIWA+hulEP4doTwFCx9GVpxg4A6W2BUWkTXYNuqr5 7/w5nWV1CkLi8AC2h4tmfsTCjhSitdtl0T0GIPVVJ8no3U0MrX/XJV9nBC4IrhBA8fif i9Q+5dTEH09gjQ6mw5vtcurpslb2a1w/JG3zWaLKWUEpzIMHHu+zidIJsgT0l5/fWTHL xPKA== X-Gm-Message-State: AFqh2kqnlBwZc0JAhXQxGKgOvvocLZgIBAk4hfn+bOJxxd9WX3CZ4DQg kFYuXKXZlOkVmEpYwDYZFML/io8YbK1j1/KCf8U= X-Google-Smtp-Source: AMrXdXtMTox8B46ALIo4syf+7HtsKRbVhPvSDri+CMEdGOuwWo7hFBx13rDIPTcVjDqhksiad0KK2Sbct4EEnrA7q6c= X-Received: by 2002:a05:6830:1103:b0:66e:7214:c7dd with SMTP id w3-20020a056830110300b0066e7214c7ddmr2721291otq.82.1672844730716; Wed, 04 Jan 2023 07:05:30 -0800 (PST) MIME-Version: 1.0 References: <20220707222015.GD13040@telsasoft.com> <2954811.1667711530@sss.pgh.pa.us> In-Reply-To: <2954811.1667711530@sss.pgh.pa.us> From: Ibrar Ahmed Date: Wed, 4 Jan 2023 20:04:53 +0500 Message-ID: Subject: Re: explain analyze rows=%.0f To: Tom Lane Cc: Robert Haas , Amit Kapila , Justin Pryzby , Peter Geoghegan , Greg Stark , vignesh C , "David G. Johnston" , pgsql-hackers@lists.postgresql.org Content-Type: multipart/alternative; boundary="000000000000f054b905f171838c" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000f054b905f171838c Content-Type: text/plain; charset="UTF-8" On Sun, Nov 6, 2022 at 10:12 AM Tom Lane wrote: > Robert Haas writes: > > On Fri, Jul 22, 2022 at 6:47 AM Amit Kapila > 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 --000000000000f054b905f171838c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Sun, Nov 6, 2022 at 10:12 AM Tom = Lane <tgl@sss.pgh.pa.us> wro= te:
Robert Haas = <robertmhaas@= gmail.com> writes:
> On Fri, Jul 22, 2022 at 6:47 AM Amit Kapila <amit.kapila16@gmail.com> wr= =3D
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<= br> > 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
=C2=A0
I took a brief look through the patch, and I have some review
comments:

* Code like this is pretty awful:

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 appendStringInfo(es= ->str,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(nloops =3D3D=3D3D 1 || !HAS_D= ECIMAL(rows)) ?
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0" rows=3D3D%.0f loops=3D3= D%.0f)" : " rows=3D3D%=3D
.2f loops=3D3D%.0f)",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rows, nloops);

Don't use variable format strings.=C2=A0 They're hard to read and t= hey
probably defeat compile-time checks that the arguments match the
format string.=C2=A0 You could use a "*" field width instead, ie<= br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 appendStringInfo(es= ->str,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0" rows=3D3D%.*f loops=3D3= D%.0f)",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(nloops =3D3D=3D3D 1 || !HAS_D= ECIMAL(rows)) ?=3D
=C2=A02 : 0,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rows, 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.=C2=A0 I gather that an earlier version of the pat= ch
tried to check whether the fractional part would be zero to two decimal
places, rather than whether it's exactly zero.=C2=A0 Probably want to p= ut
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.=C2=A0

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


=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 regards, tom lane


--
Ibrar Ahmed
--000000000000f054b905f171838c--