On Tue, May 14, 2024 at 6:33 PM David Rowley <dgrowle...@gmail.com> wrote:
>
> On Tue, 14 May 2024 at 18:16, David Rowley <dgrowle...@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.
