Re: explain analyze rows=%.0f

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> wrote:
>>
>> On Wed, Jun 22, 2022 at 12:11 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
>>>
>>> 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@2ndQuadrant.com> 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
entiresection not just a modifier for the word "rows".  This is evidenced by the timing block verbiage where rows is
standaloneand 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'thave 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.

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;

Regards,
Vignesh