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 1o9sRf-0004az-79 for pgsql-hackers@arkaria.postgresql.org; Fri, 08 Jul 2022 18:11:15 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1o9sRe-0001wv-29 for pgsql-hackers@arkaria.postgresql.org; Fri, 08 Jul 2022 18:11:14 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1o9sRd-0001qz-GK for pgsql-hackers@lists.postgresql.org; Fri, 08 Jul 2022 18:11:13 +0000 Received: from mail-vk1-xa34.google.com ([2607:f8b0:4864:20::a34]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1o9sRa-0001dU-6L for pgsql-hackers@postgresql.org; Fri, 08 Jul 2022 18:11:12 +0000 Received: by mail-vk1-xa34.google.com with SMTP id 18so5186166vkz.2 for ; Fri, 08 Jul 2022 11:11:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=kW/tcKF0zh2dVISd4v7xHFb1L/HlQlJ6DmO39QSpkCI=; b=cU71ehrMPxq177xCeCQSAE8HscdJMXMRWzlQaPazGcp8i8HOCLxTuYJkR0NarCdDKl YohIKM3lnUhQu6oH0jrK4Zkym7rAJef/LzddmZmjQbDDT42ys7QYGCtL+Fx843DDYopl BJyosHJR8m9lsY4XpUnu9HJNz+CJQL8G59xf6XLUVP9gj1r4yVmd4v+tfCF4m3P7l2Jn noc8ZBT5WfyYSxW3/k4Pd4RcIZZOv7pYSo5si4n36xxLyJrV0L+LJ6fgCwd7ij95eOnK CWVH3ZMsYmkZ1l1bEHPdauIJK1xkXq42XnViSj/PjJZi2W++uDSB98eFZSl9rQ6Nm7kj Jkmw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=kW/tcKF0zh2dVISd4v7xHFb1L/HlQlJ6DmO39QSpkCI=; b=602lrYn29UvUlXa3xYd7Gp5lEMZTV+ULOy3mSOg4dNywv2sO/lSeeYDsDQBEvAn7Za CuONN5lwgtw6x/EUa7rQhhxzy5aZMsJpbIPnAmgxplj1nEnUOTTkhSJxBbxayOWSUvR/ scn5AhlhYx1/LMvI3tguI0fZ+VcOZNAha0+Fm4aE+LpBebqcR6Ai8Sk9VDE/BtLLHSlu zovdhWmYQIQSvOW9yLyLrtWyB5w8hVhQyjXtjYH5r4dcBFPwk6DYY3x9oZ8AEFd3UyJ+ ZuFocqtjZHbQgZCvgX4xjvc+p/PEoDMkgijgBuIVqm9aGDG+L6r+r/1bAGk36kpLdR4m LyWA== X-Gm-Message-State: AJIora9NiNbw6m5hMqPReIJWswZH/RiswoUu6mXxeEcpuQ+ze9No2XMt Hj6AbnK8cRxr2Ae+8HkeLlMtmYw7sOfMW6f0caCxvU4vfZ1hvUO2 X-Google-Smtp-Source: AGRyM1tNE7OnmoXJ/5+/jj7Rr61jOQSSkel5l5e5BvC/giB4eh4xlnHUpc8Vv7e6ql1aDaly2UXcTVpLkS6hfIkBq5o= X-Received: by 2002:a1f:a546:0:b0:374:5cd:93da with SMTP id o67-20020a1fa546000000b0037405cd93damr2318050vke.5.1657303869163; Fri, 08 Jul 2022 11:11:09 -0700 (PDT) MIME-Version: 1.0 References: <603c8f070905281830g2e5419c4xad2946d149e21f9d@mail.gmail.com> <4A1F4FD5.7060204@timbira.com> <4A249CE9.6050708@cheapcomplexdevices.com> <1243950060.23910.58.camel@ebony.2ndQuadrant> <11926.1243953507@sss.pgh.pa.us> In-Reply-To: From: Ibrar Ahmed Date: Fri, 8 Jul 2022 23:10:31 +0500 Message-ID: Subject: Re: explain analyze rows=%.0f To: vignesh C Cc: "David G. Johnston" , "pgsql-hackers@postgresql.org" , Robert Haas , Simon Riggs , Ron Mayer , Euler Taveira de Oliveira , Tom Lane Content-Type: multipart/alternative; boundary="000000000000680ec905e34f206f" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000680ec905e34f206f Content-Type: text/plain; charset="UTF-8" On Thu, Jul 7, 2022 at 3:14 PM vignesh C wrote: > On Thu, Jun 23, 2022 at 2:25 AM Ibrar Ahmed wrote: > > > > > > > > On Thu, Jun 23, 2022 at 1:04 AM David G. Johnston < > david.g.johnston@gmail.com> wrote: > >> > >> On Wed, Jun 22, 2022 at 12:11 PM Ibrar Ahmed > wrote: > >>> > >>> On Thu, Jun 23, 2022 at 12:01 AM Tom Lane wrote: > >>>> > >>>> Robert Haas writes: > >>>> > On Jun 2, 2009, at 9:41 AM, Simon Riggs > wrote: > >>>> >> You're right that the number of significant digits already exceeds > the > >>>> >> true accuracy of the computation. I think what Robert wants to see > is > >>>> >> the exact value used in the calc, so the estimates can be checked > more > >>>> >> thoroughly than is currently possible. > >>>> > >>>> > Bingo. > >>>> > >>>> Uh, the planner's estimate *is* an integer. What was under discussion > >>>> (I thought) was showing some fractional digits in the case where > EXPLAIN > >>>> ANALYZE is outputting a measured row count that is an average over > >>>> multiple loops, and therefore isn't necessarily an integer. In that > >>>> case the measured value can be considered arbitrarily precise --- > though > >>>> I think in practice one or two fractional digits would be plenty. > >>>> > >>>> regards, tom lane > >>>> > >>>> > >>> Hi, > >>> I was looking at the TODO list and found that the issue requires > >>> a quick fix. Attached is a patch which shows output like this. > >> > >> > >> Quick code review: > >> > >> + "actual rows=%.0f loops=%.0f": " rows=%.2f loops=%.0f", > >> > >> The leading space before the else block "rows" does not belong. > >> > >> There should be a space after the colon. > >> > > Thanks, David for your quick response. I have updated the patch. > > > >> > >> The word "actual" that you are dropping in the else block seems like it > should belong - it is a header for the entire section not just a modifier > for the word "rows". This is evidenced by the timing block verbiage where > rows is standalone and the word actual comes before time. In short, only > the format specifier should change under the current scheme. Both sections. > >> > >> - WRITE_FLOAT_FIELD(rows, "%.0f"); > >> + WRITE_FLOAT_FIELD(rows, "%.2f"); > >> > >> This one looks suspicious, though I haven't dug into the code to see > exactly what all is being touched. That it doesn't have an nloops > condition like everything else stands out. > >> > > I was also thinking about that, but I don't see any harm when we > ultimately truncating that decimal > > at a latter stage of code in case of loop = 1. > > Thanks for the patch. > Thanks for the review. > > 1) There are some existing regression tests that are failing, you > should update the expect files accordingly for the same: > --- /home/vignesh/postgres/src/test/regress/expected/select_parallel.out > 2022-05-18 20:51:46.874818044 +0530 > +++ /home/vignesh/postgres/src/test/regress/results/select_parallel.out > 2022-07-07 15:27:34.450440922 +0530 > @@ -545,17 +545,17 @@ > explain (analyze, timing off, summary off, costs off) > select count(*) from tenk1, tenk2 where tenk1.hundred > 1 > and tenk2.thousand=0; > - QUERY PLAN > --------------------------------------------------------------------------- > + QUERY PLAN > > +----------------------------------------------------------------------------- > Aggregate (actual rows=1 loops=1) > -> Nested Loop (actual rows=98000 loops=1) > -> Seq Scan on tenk2 (actual rows=10 loops=1) > Filter: (thousand = 0) > Rows Removed by Filter: 9990 > - -> Gather (actual rows=9800 loops=10) > + -> Gather (actual rows=9800.00 loops=10) > Workers Planned: 4 > Workers Launched: 4 > - -> Parallel Seq Scan on tenk1 (actual rows=1960 loops=50) > + -> Parallel Seq Scan on tenk1 (actual rows=1960.00 > loops=50) > Filter: (hundred > 1) > > test select_parallel ... FAILED 744 ms > partition_prune ... FAILED 861 ms > explain ... FAILED 134 ms > memoize ... FAILED 250 ms > > 2) This change is not required as part of this patch: > --- a/src/backend/access/transam/xact.c > +++ b/src/backend/access/transam/xact.c > @@ -122,7 +122,7 @@ bool bsysscan = false; > * lookups as fast as possible. > */ > static FullTransactionId XactTopFullTransactionId = > {InvalidTransactionId}; > -static int nParallelCurrentXids = 0; > +static int nParallelCurrentXids = 0; > static TransactionId *ParallelCurrentXids; > > I have fixed the regression and removed non-related code. > Regards, > Vignesh > -- Ibrar Ahmed --000000000000680ec905e34f206f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Thu, Jul 7, 2022 at 3:14 PM vigne= sh C <vignesh21@gmail.com>= wrote:
On Thu, = Jun 23, 2022 at 2:25 AM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
>
>
>
> On Thu, Jun 23, 2022 at 1:04 AM David G. Johnston <david.g.johnston@gmail.com<= /a>> wrote:
>>
>> On Wed, Jun 22, 2022 at 12:11 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wro= te:
>>>
>>> On Thu, Jun 23, 2022 at 12:01 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>
>>>> Robert Haas <robertmhaas@gmail.com> writes:
>>>> > On Jun 2, 2009, at 9:41 AM, Simon Riggs <simon@2nd= Quadrant.com> wrote:
>>>> >> You're right that the number of significant d= igits already exceeds the
>>>> >> true accuracy of the computation. I think what Ro= bert wants to see is
>>>> >> the exact value used in the calc, so the estimate= s can be checked more
>>>> >> thoroughly than is currently possible.
>>>>
>>>> > Bingo.
>>>>
>>>> Uh, the planner's estimate *is* an integer.=C2=A0 What= was under discussion
>>>> (I thought) was showing some fractional digits in the case= where EXPLAIN
>>>> ANALYZE is outputting a measured row count that is an aver= age over
>>>> multiple loops, and therefore isn't necessarily an int= eger.=C2=A0 In that
>>>> case the measured value can be considered arbitrarily prec= ise --- though
>>>> I think in practice one or two fractional digits would be = plenty.
>>>>
>>>>=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=A0regards, tom lane
>>>>
>>>>
>>> Hi,
>>> I was looking at the TODO list and found that the issue requir= es
>>> a quick fix. Attached is a patch which shows output like this.=
>>
>>
>> Quick code review:
>>
>> + "actual rows=3D%.0f loops=3D%.0f": " rows=3D%.2f = loops=3D%.0f",
>>
>> The leading space before the else block "rows" does not = belong.
>>
>> There should be a space after the colon.
>>
> Thanks, David for your quick response. I have updated the patch.
>
>>
>> The word "actual" that you are dropping in the else bloc= k seems like it should belong - it is a header for the entire section not j= ust a modifier for the word "rows".=C2=A0 This is evidenced by th= e timing block verbiage where rows is standalone and the word actual comes = before time.=C2=A0 In short, only the format specifier should change under = the current scheme.=C2=A0 Both sections.
>>
>> - WRITE_FLOAT_FIELD(rows, "%.0f");
>> + WRITE_FLOAT_FIELD(rows, "%.2f");
>>
>> This one looks suspicious, though I haven't dug into the code = to see exactly what all is being touched.=C2=A0 That it doesn't have an= nloops condition like everything else stands out.
>>
> I was also thinking about that, but I don't see any harm when we u= ltimately truncating that decimal
> at a latter stage of code in case of loop =3D 1.

Thanks for the patch.

Thanks for the review.

1) There are some existing regression tests that are failing, you
should update the expect files accordingly for the same:
--- /home/vignesh/postgres/src/test/regress/expected/select_parallel.out =C2=A0 =C2=A0 =C2=A0 =C2=A02022-05-18 20:51:46.874818044 +0530
+++ /home/vignesh/postgres/src/test/regress/results/select_parallel.out
2022-07-07 15:27:34.450440922 +0530
@@ -545,17 +545,17 @@
=C2=A0explain (analyze, timing off, summary off, costs off)
=C2=A0 =C2=A0 select count(*) from tenk1, tenk2 where tenk1.hundred > 1<= br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0and tenk2.thousand=3D0;
-=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 QUERY PLAN
---------------------------------------------------------------------------=
+=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=A0QUERY PLAN
+--------------------------------------------------------------------------= ---
=C2=A0 Aggregate (actual rows=3D1 loops=3D1)
=C2=A0 =C2=A0 ->=C2=A0 Nested Loop (actual rows=3D98000 loops=3D1)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ->=C2=A0 Seq Scan on tenk2 (actual ro= ws=3D10 loops=3D1)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Filter: (thousand = =3D 0)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Rows Removed by Fil= ter: 9990
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0->=C2=A0 Gather (actual rows=3D9800 l= oops=3D10)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0->=C2=A0 Gather (actual rows=3D9800.0= 0 loops=3D10)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Workers Planned: 4<= br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Workers Launched: 4=
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0->=C2=A0 Paralle= l Seq Scan on tenk1 (actual rows=3D1960 loops=3D50)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0->=C2=A0 Paralle= l Seq Scan on tenk1 (actual rows=3D1960.00 loops=3D50)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 Filter: (hundred > 1)

=C2=A0 =C2=A0 test select_parallel=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 ... FAILED=C2=A0 =C2=A0 =C2=A0 744 ms
=C2=A0 =C2=A0 =C2=A0partition_prune=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 ... FAILED=C2=A0 =C2=A0 =C2=A0 861 ms
=C2=A0 =C2=A0 =C2=A0explain=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 ... FAILED=C2=A0 =C2=A0 =C2=A0 134 ms
=C2=A0 =C2=A0 =C2=A0memoize=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 ... FAILED=C2=A0 =C2=A0 =C2=A0 250 ms

2) This change is not required as part of this patch:
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -122,7 +122,7 @@ bool=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 bsysscan =3D false;
=C2=A0 * lookups as fast as possible.
=C2=A0 */
=C2=A0static FullTransactionId XactTopFullTransactionId =3D {InvalidTransac= tionId};
-static int nParallelCurrentXids =3D 0;
+static int=C2=A0 =C2=A0 =C2=A0nParallelCurrentXids =3D 0;
=C2=A0static TransactionId *ParallelCurrentXids;


I have fixed the regression and removed non-rela= ted code.
Regards,
Vignesh


--
Ibrar Ahmed
--000000000000680ec905e34f206f--