RE: Use of "long" in incremental sort code

Hi

Found one more place needed to be changed(long -> int64).

Also changed the output for int64 data(Debug mode on & define EXEC_SORTDEBUG )

And, maybe there's a typo in " src\backend\executor\nodeIncrementalSort.c" as below.
Obviously, the ">=" is meaningless, right?

And, maybe there's a typo in " src\backend\executor\nodeIncrementalSort.c" as below.
Obviously, the ">=" is meaningless, right?

-		SO1_printf("Sorting presorted prefix tuplesort with >= %ld tuples\n", nTuples);
+		SO1_printf("Sorting presorted prefix tuplesort with %ld tuples\n", nTuples);

Please take a check at the attached patch file.

I have added it to commit fest.
https://commitfest.postgresql.org/30/2772/

Best regards
Tang

-----Original Message-----
From: Tang, Haiying <tanghy.fnst@cn.fujitsu.com>
Sent: Monday, October 19, 2020 12:57 PM
To: David Rowley <dgrowleyml@gmail.com>; James Coleman <jtc331@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: RE: Use of "long" in incremental sort code

Hi

Found one more place needed to be changed(long -> int64).

Also changed the output for int64 data(Debug mode on & define EXEC_SORTDEBUG )

And, maybe there's a typo in " src\backend\executor\nodeIncrementalSort.c" as below.
Obviously, the ">=" is meaningless, right?

-		SO1_printf("Sorting presorted prefix tuplesort with >= %ld tuples\n", nTuples);
+		SO1_printf("Sorting presorted prefix tuplesort with %ld tuples\n", nTuples);

Please take a check at the attached patch file.

Previous disscution:
/messages/by-id/CAApHDvpky+Uhof8mryPf5i=6e6fib2dxHqBrhp0Qhu0NeBhLJw@mail.gmail.com

Best regards
Tang

On Wed, Oct 21, 2020 at 06:06:52AM +0000, Tang, Haiying wrote:

Hi

Found one more place needed to be changed(long -> int64).

Also changed the output for int64 data(Debug mode on & define EXEC_SORTDEBUG )

And, maybe there's a typo in " src\backend\executor\nodeIncrementalSort.c" as below.
Obviously, the ">=" is meaningless, right?

And, maybe there's a typo in " src\backend\executor\nodeIncrementalSort.c" as below.
Obviously, the ">=" is meaningless, right?

-		SO1_printf("Sorting presorted prefix tuplesort with >= %ld tuples\n", nTuples);
+		SO1_printf("Sorting presorted prefix tuplesort with %ld tuples\n", nTuples);

Please take a check at the attached patch file.

I have added it to commit fest.
https://commitfest.postgresql.org/30/2772/

Thanks, the changes seem fine to me. I'll do a bit more review and get
it pushed.

regards
Tomas

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.

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 ...

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Wed, 4 Nov 2020 at 10:42, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:

IMHO this should simply switch the current int64 variable to long, as it
was before. Not sure about about the hashagg uint64 variable.

IMO, we should just get rid of the use of "long" here. As far as I'm
concerned, using long in the core code at all is just unnecessary and
just increases the chances of having bugs.

How often do people forget that we support a 64-bit platform that has
sizeof(long) == 4?

Can't we use size_t and ssize_t if we really need a processor
word-sized type? And use int64/uint64 when we really want a 64-bit
type.

David

On 11/4/20 10:58 PM, David Rowley wrote:

On Wed, 4 Nov 2020 at 10:42, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:

IMHO this should simply switch the current int64 variable to long, as it
was before. Not sure about about the hashagg uint64 variable.

IMO, we should just get rid of the use of "long" here. As far as I'm
concerned, using long in the core code at all is just unnecessary and
just increases the chances of having bugs.

How often do people forget that we support a 64-bit platform that has
sizeof(long) == 4?

Can't we use size_t and ssize_t if we really need a processor
word-sized type? And use int64/uint64 when we really want a 64-bit
type.

Perhaps. But I guess it's a bit strange to have function declared as
returning long, but store the result in int64 everywhere. That was the
point I was trying to make - it's not just a matter of changing all the
variables to int64, IMHO.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 05.11.2020 02:53, James Coleman wrote:

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?

At first glance, I haven't found anything that could limit tape code. It
uses BufFile, which is not limited by the OS file size limit.
Still, If we want to change 'long' in LogicalTapeSetBlocks, we should
probably also update nBlocksWritten and other variables.

As far as I see, the major part of the patch was committed, so l update
the status of the CF entry to "Committed". Feel free to create a new
entry, if you're going to continue working on the remaining issue.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company