On Tue, Nov 3, 2020 at 4:42 PM Tomas Vondra <tomas.von...@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
