On Tue, Nov 3, 2020 at 4:42 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> 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.
Is there anything that actually limits tape code to using at most 4GB
on 32-bit systems?
James