Re: explain format json, unit for serialize and memory are different. - Mailing list pgsql-hackers

On Tue, May 14, 2024 at 6:33 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Tue, 14 May 2024 at 18:16, David Rowley <dgrowleyml@gmail.com> wrote:
> > I think for v17, we should consider adding a macro to explain.c to
> > calculate the KB from bytes.  There are other inconsistencies that it
> > would be good to address. We normally round up to the nearest kilobyte
> > with (bytes + 1023) / 1024, but if you look at what 06286709e did, it
> > seems to be rounding to the nearest without any apparent justification
> > as to why.  It does (metrics->bytesSent + 512) / 1024.
> >
> > show_memory_counters() could be modified to use the macro and show
> > kilobytes rather than bytes.
>
> Here's a patch for that.
>
> I checked the EXPLAIN SERIALIZE thread and didn't see any mention of
> the + 512 thing. It seems Tom added it just before committing and no
> patch ever made it to the mailing list with + 512. The final patch on
> the list is in [1].
>
> For the EXPLAIN MEMORY part, the bytes vs kB wasn't discussed.  The
> closest the thread came to that was what Abhijit mentioned in [2].



static void
show_memory_counters(ExplainState *es, const MemoryContextCounters
*mem_counters)
{
if (es->format == EXPLAIN_FORMAT_TEXT)
{
ExplainIndentText(es);
appendStringInfo(es->str,
"Memory: used=%zukB  allocated=%zukB",
BYTES_TO_KILOBYTES(mem_counters->totalspace - mem_counters->freespace),
BYTES_TO_KILOBYTES(mem_counters->totalspace));
appendStringInfoChar(es->str, '\n');
}
else
{
ExplainPropertyInteger("Memory Used", "bytes",
   mem_counters->totalspace - mem_counters->freespace,
   es);
ExplainPropertyInteger("Memory Allocated", "bytes",
   mem_counters->totalspace, es);
}
}

the "else" branch, also need to apply BYTES_TO_KILOBYTES marco?
otherwise, it's inconsistent?

> I also adjusted some inconsistencies around spaces between the digits
> and kB.  In other places in EXPLAIN we write "100kB" not "100 kB".  I
> see we print times with a space ("Execution Time: 1.719 ms"), so we're
> not very consistent overall, but since the EXPLAIN MEMORY is new, it
> makes sense to change it now to be aligned to the other kB stuff in
> explain.c
>
> The patch does change a long into an int64 in show_hash_info(). I
> wondered if that part should be backpatched.  It does not seem very
> robust to me to divide a Size by 1024 and expect it to fit into a
> long. With MSVC 64 bit, sizeof(Size) == 8 and sizeof(long) == 4.  I
> understand work_mem is limited to 2GB on that platform, but it does
> not seem like a good reason to use a long.
>

I also checked output
from function show_incremental_sort_group_info and show_sort_info,
the "kB" usage is consistent.