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].
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.
David
[1] https://www.postgresql.org/message-id/CAEze2WhopAFRS4xdNtma6XtYCRqydPWAg83jx8HZTowpeXzOyg%40mail.gmail.com
[2] https://www.postgresql.org/message-id/ZaF1fB_hMqycSq-S%40toroid.org