Re: explain analyze rows=%.0f

On Thu, Jul 7, 2022 at 2:41 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Thu, Jun 23, 2022 at 2:25 AM Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com> wrote:
> >
> > On Thu, Jun 23, 2022 at 1:04 AM David G. Johnston <
> david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
> >>
> >> - 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.
> >
>
> That change is in the path node which we anyway not going to target as
> part of this change. We only want to change the display for actual
> rows in Explain Analyze. So, I can't see how the quoted change can
> help in any way.
>
> Agreed removed.

> Few miscellaneous comments:
> ========================
> *
> static FullTransactionId XactTopFullTransactionId =
> {InvalidTransactionId};
> -static int nParallelCurrentXids = 0;
> +static int nParallelCurrentXids = 0;
>
> Removed.

> I don't see why this change is required.
>
> * Can you please add a comment explaining why we are making this
> change for actual rows?
>

Done

>
> * Can you please write a test case unless there is some existing test
> that covers the change by displaying actual rows values in decimal but
> in that case patch should have that changed output test? If you don't
> think we can reliably write such a test then please let me know the
> reason?
>
> I think there are tests, and I have updated the results accordingly.

> --
> With Regards,
> Amit Kapila.
>

--
Ibrar Ahmed