On Tue, Nov 03, 2020 at 03:53:53AM +0100, Tomas Vondra wrote:
>Hi,
>
>I took another look at this, and 99% of the patch (the fixes to sort
>debug messages) seems fine to me. Attached is the part I plan to get
>committed, including commit message etc.
>
I've pushed this part. Thanks for the patch, Haiying Tang.
>
>The one change I decided to remove is this change in tuplesort_free:
>
>- long spaceUsed;
>+ int64 spaceUsed;
>
>The reason why I think this variable should be 'long' is that we're
>using it for this:
>
> spaceUsed = LogicalTapeSetBlocks(state->tapeset);
>
>and LogicalTapeSetBlocks is defined like this:
>
> extern long LogicalTapeSetBlocks(LogicalTapeSet *lts);
>
>FWIW the "long" is not introduced by incremental sort - it used to be in
>tuplesort_end, the incremental sort patch just moved it to a different
>function. It's a bit confusing that tuplesort_updatemax has this:
>
> int64 spaceUsed;
>
>But I'd argue this is actually wrong, and should be "long" instead. (And
>this actually comes from the incremental sort patch, by me.)
>
>
>FWIW while looking at what the other places calling LogicalTapeSetBlocks
>do, and I noticed this:
>
> uint64 disk_used = LogicalTapeSetBlocks(...);
>
>in the disk-based hashagg patch. So that's a third data type ...
>
IMHO this should simply switch the current int64 variable to long, as it
was before. Not sure about about the hashagg uint64 variable.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
