Use of "long" in incremental sort code
Hi,
I noticed the incremental sort code makes use of the long datatype a
few times, e.g in TuplesortInstrumentation and
IncrementalSortGroupInfo. (64-bit windows machines have sizeof(long)
== 4). I understand that the values are in kilobytes and it would
take 2TB to cause them to wrap. Never-the-less, I think it would be
better to choose a better-suited type. work_mem is still limited to
2GB on 64-bit Windows machines, so perhaps there's some argument that
it does not matter about fields that related to in-memory stuff, but
the on-disk fields are wrong. The in-memory fields likely raise the
bar further for fixing the 2GB work_mem limit on Windows.
Maybe Size would be better for the in-memory fields and uint64 for the
on-disk fields?
David
David Rowley <dgrowleyml@gmail.com> writes:
I noticed the incremental sort code makes use of the long datatype a
few times, e.g in TuplesortInstrumentation and
IncrementalSortGroupInfo. (64-bit windows machines have sizeof(long)
== 4). I understand that the values are in kilobytes and it would
take 2TB to cause them to wrap. Never-the-less, I think it would be
better to choose a better-suited type. work_mem is still limited to
2GB on 64-bit Windows machines, so perhaps there's some argument that
it does not matter about fields that related to in-memory stuff, but
the on-disk fields are wrong. The in-memory fields likely raise the
bar further for fixing the 2GB work_mem limit on Windows.
Maybe Size would be better for the in-memory fields and uint64 for the
on-disk fields?
There is a fairly widespread issue that memory-size-related GUCs and
suchlike variables are limited to represent sizes that fit in a "long".
Although Win64 is the *only* platform where that's an issue, maybe
it's worth doing something about. But we shouldn't just fix the sort
code, if we do do something.
(IOW, I don't agree with doing a fix that doesn't also fix work_mem.)
regards, tom lane
On Tue, 30 Jun 2020 at 16:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
There is a fairly widespread issue that memory-size-related GUCs and
suchlike variables are limited to represent sizes that fit in a "long".
Although Win64 is the *only* platform where that's an issue, maybe
it's worth doing something about. But we shouldn't just fix the sort
code, if we do do something.(IOW, I don't agree with doing a fix that doesn't also fix work_mem.)
I raised it mostly because this new-to-PG13-code is making the problem worse.
If we're not going to change the in-memory fields, then shouldn't we
at least change the ones for disk space tracking?
David
On 2020-06-30 06:24, David Rowley wrote:
On Tue, 30 Jun 2020 at 16:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
There is a fairly widespread issue that memory-size-related GUCs and
suchlike variables are limited to represent sizes that fit in a "long".
Although Win64 is the *only* platform where that's an issue, maybe
it's worth doing something about. But we shouldn't just fix the sort
code, if we do do something.(IOW, I don't agree with doing a fix that doesn't also fix work_mem.)
I raised it mostly because this new-to-PG13-code is making the problem worse.
Yeah, we recently got rid of a bunch of inappropriate use of long, so it
seems reasonable to make this new code follow that.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jun 30, 2020 at 7:21 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 2020-06-30 06:24, David Rowley wrote:
On Tue, 30 Jun 2020 at 16:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
There is a fairly widespread issue that memory-size-related GUCs and
suchlike variables are limited to represent sizes that fit in a "long".
Although Win64 is the *only* platform where that's an issue, maybe
it's worth doing something about. But we shouldn't just fix the sort
code, if we do do something.(IOW, I don't agree with doing a fix that doesn't also fix work_mem.)
I raised it mostly because this new-to-PG13-code is making the problem worse.
Yeah, we recently got rid of a bunch of inappropriate use of long, so it
seems reasonable to make this new code follow that.
I've attached a patch to make this change but with one tweak: I
decided to use unint64 for both memory and disk (rather than Size in
some cases) since we aggregated across multiple runs and have shared
code that deals with both values.
James
Attachments:
v1-0001-Use-unint64-instead-of-long-for-space-used-variab.patchapplication/octet-stream; name=v1-0001-Use-unint64-instead-of-long-for-space-used-variab.patchDownload
From f7c22472a9b2b3aca5bb5c4f803ae17b493c39f2 Mon Sep 17 00:00:00 2001
From: jcoleman <jtc331@gmail.com>
Date: Thu, 2 Jul 2020 15:59:14 +0000
Subject: [PATCH v1] Use unint64 instead of long for space used variables in
incremental sort
---
src/backend/commands/explain.c | 20 ++++++++++----------
src/include/nodes/execnodes.h | 8 ++++----
src/include/utils/tuplesort.h | 2 +-
3 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 093864cfc0..ff70a1ab04 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2676,7 +2676,7 @@ show_sort_info(SortState *sortstate, ExplainState *es)
TuplesortInstrumentation stats;
const char *sortMethod;
const char *spaceType;
- long spaceUsed;
+ uint64 spaceUsed;
tuplesort_get_stats(state, &stats);
sortMethod = tuplesort_method_name(stats.sortMethod);
@@ -2686,7 +2686,7 @@ show_sort_info(SortState *sortstate, ExplainState *es)
if (es->format == EXPLAIN_FORMAT_TEXT)
{
ExplainIndentText(es);
- appendStringInfo(es->str, "Sort Method: %s %s: %ldkB\n",
+ appendStringInfo(es->str, "Sort Method: %s %s: "UINT64_FORMAT "kB\n",
sortMethod, spaceType, spaceUsed);
}
else
@@ -2715,7 +2715,7 @@ show_sort_info(SortState *sortstate, ExplainState *es)
TuplesortInstrumentation *sinstrument;
const char *sortMethod;
const char *spaceType;
- long spaceUsed;
+ uint64 spaceUsed;
sinstrument = &sortstate->shared_info->sinstrument[n];
if (sinstrument->sortMethod == SORT_TYPE_STILL_IN_PROGRESS)
@@ -2731,7 +2731,7 @@ show_sort_info(SortState *sortstate, ExplainState *es)
{
ExplainIndentText(es);
appendStringInfo(es->str,
- "Sort Method: %s %s: %ldkB\n",
+ "Sort Method: %s %s: "UINT64_FORMAT "kB\n",
sortMethod, spaceType, spaceUsed);
}
else
@@ -2795,23 +2795,23 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
if (groupInfo->maxMemorySpaceUsed > 0)
{
- long avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount;
+ uint64 avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount;
const char *spaceTypeName;
spaceTypeName = tuplesort_space_type_name(SORT_SPACE_TYPE_MEMORY);
- appendStringInfo(es->str, " Average %s: %ldkB Peak %s: %ldkB",
+ appendStringInfo(es->str, " Average %s: " UINT64_FORMAT "kB Peak %s: " UINT64_FORMAT "kB",
spaceTypeName, avgSpace,
spaceTypeName, groupInfo->maxMemorySpaceUsed);
}
if (groupInfo->maxDiskSpaceUsed > 0)
{
- long avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
+ uint64 avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
const char *spaceTypeName;
spaceTypeName = tuplesort_space_type_name(SORT_SPACE_TYPE_DISK);
- appendStringInfo(es->str, " Average %s: %ldkB Peak %s: %ldkB",
+ appendStringInfo(es->str, " Average %s: " UINT64_FORMAT "kB Peak %s: " UINT64_FORMAT "kB",
spaceTypeName, avgSpace,
spaceTypeName, groupInfo->maxDiskSpaceUsed);
}
@@ -2829,7 +2829,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
if (groupInfo->maxMemorySpaceUsed > 0)
{
- long avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount;
+ uint64 avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount;
const char *spaceTypeName;
StringInfoData memoryName;
@@ -2846,7 +2846,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
}
if (groupInfo->maxDiskSpaceUsed > 0)
{
- long avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
+ uint64 avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
const char *spaceTypeName;
StringInfoData diskName;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index f5dfa32d55..2cd016ac6d 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2032,10 +2032,10 @@ typedef struct SortState
typedef struct IncrementalSortGroupInfo
{
int64 groupCount;
- long maxDiskSpaceUsed;
- long totalDiskSpaceUsed;
- long maxMemorySpaceUsed;
- long totalMemorySpaceUsed;
+ uint64 maxDiskSpaceUsed;
+ uint64 totalDiskSpaceUsed;
+ uint64 maxMemorySpaceUsed;
+ uint64 totalMemorySpaceUsed;
bits32 sortMethods; /* bitmask of TuplesortMethod */
} IncrementalSortGroupInfo;
diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h
index d992b4875a..72b41b5fec 100644
--- a/src/include/utils/tuplesort.h
+++ b/src/include/utils/tuplesort.h
@@ -90,7 +90,7 @@ typedef struct TuplesortInstrumentation
{
TuplesortMethod sortMethod; /* sort algorithm used */
TuplesortSpaceType spaceType; /* type of space spaceUsed represents */
- long spaceUsed; /* space consumption, in kB */
+ uint64 spaceUsed; /* space consumption, in kB */
} TuplesortInstrumentation;
--
2.20.1
On Mon, Jun 29, 2020 at 9:13 PM David Rowley <dgrowleyml@gmail.com> wrote:
I noticed the incremental sort code makes use of the long datatype a
few times, e.g in TuplesortInstrumentation and
IncrementalSortGroupInfo.
I agree that long is terrible, and should generally be avoided.
Maybe Size would be better for the in-memory fields and uint64 for the
on-disk fields?
FWIW we have to use int64 for the in-memory tuplesort.c fields. This
is because it must be possible for the fields to have negative values
in the context of tuplesort. If there is going to be a general rule
for in-memory fields, then ISTM that it'll have to be "use int64".
logtape.c uses long for on-disk fields. It also relies on negative
values, albeit to a fairly limited degree (it uses -1 as a magic
value).
--
Peter Geoghegan
On Thu, Jul 2, 2020 at 1:36 PM Peter Geoghegan <pg@bowt.ie> wrote:
On Mon, Jun 29, 2020 at 9:13 PM David Rowley <dgrowleyml@gmail.com> wrote:
I noticed the incremental sort code makes use of the long datatype a
few times, e.g in TuplesortInstrumentation and
IncrementalSortGroupInfo.I agree that long is terrible, and should generally be avoided.
Maybe Size would be better for the in-memory fields and uint64 for the
on-disk fields?FWIW we have to use int64 for the in-memory tuplesort.c fields. This
is because it must be possible for the fields to have negative values
in the context of tuplesort. If there is going to be a general rule
for in-memory fields, then ISTM that it'll have to be "use int64".logtape.c uses long for on-disk fields. It also relies on negative
values, albeit to a fairly limited degree (it uses -1 as a magic
value).
Do you think it's reasonable to use int64 across the board for memory
and disk space numbers then? If so, I can update the patch.
James
On Thu, Jul 2, 2020 at 10:53 AM James Coleman <jtc331@gmail.com> wrote:
Do you think it's reasonable to use int64 across the board for memory
and disk space numbers then? If so, I can update the patch.
Using int64 as a replacement for long is the safest general strategy,
and so ISTM that it might be worth doing that even in cases where it
isn't clearly necessary. After all, any code that uses long must have
been written with the assumption that that was the same thing as
int64, at least on most platforms.
There is nothing wrong with using Size/size_t, and doing so is often
slightly clearer. But it's no drop-in replacement for long.
--
Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
On Thu, Jul 2, 2020 at 10:53 AM James Coleman <jtc331@gmail.com> wrote:
Do you think it's reasonable to use int64 across the board for memory
and disk space numbers then? If so, I can update the patch.
Using int64 as a replacement for long is the safest general strategy,
mumble ssize_t mumble
regards, tom lane
On Thu, Jul 2, 2020 at 12:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
mumble ssize_t mumble
That's from POSIX, though. I imagine MSVC won't be happy (surprise!).
--
Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
On Thu, Jul 2, 2020 at 12:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
mumble ssize_t mumble
That's from POSIX, though. I imagine MSVC won't be happy (surprise!).
We've got quite a few uses of it already, so apparently it's fine.
regards, tom lane
On Thu, Jul 2, 2020 at 3:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Geoghegan <pg@bowt.ie> writes:
On Thu, Jul 2, 2020 at 10:53 AM James Coleman <jtc331@gmail.com> wrote:
Do you think it's reasonable to use int64 across the board for memory
and disk space numbers then? If so, I can update the patch.Using int64 as a replacement for long is the safest general strategy,
mumble ssize_t mumble
But wouldn't that mean we'd get int on 32-bit systems, and since we're
accumulating data we could go over that value in both memory and disk?
My assumption is that it's preferable to have the "this run value" and
the "total used across multiple runs" and both of those for disk and
memory to be the same. In that case it seems we want to guarantee
64-bits.
Patch using int64 attached.
James
Attachments:
v2-0001-Use-int64-instead-of-long-for-space-used-variable.patchapplication/octet-stream; name=v2-0001-Use-int64-instead-of-long-for-space-used-variable.patchDownload
From 9c33ed604408d7daf8f4e9af6e33461642457cae Mon Sep 17 00:00:00 2001
From: jcoleman <jtc331@gmail.com>
Date: Thu, 2 Jul 2020 15:59:14 +0000
Subject: [PATCH v2] Use int64 instead of long for space used variables in
incremental sort
---
src/backend/commands/explain.c | 20 ++++++++++----------
src/include/nodes/execnodes.h | 8 ++++----
src/include/utils/tuplesort.h | 2 +-
3 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 093864cfc0..d4baba3a07 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2676,7 +2676,7 @@ show_sort_info(SortState *sortstate, ExplainState *es)
TuplesortInstrumentation stats;
const char *sortMethod;
const char *spaceType;
- long spaceUsed;
+ int64 spaceUsed;
tuplesort_get_stats(state, &stats);
sortMethod = tuplesort_method_name(stats.sortMethod);
@@ -2686,7 +2686,7 @@ show_sort_info(SortState *sortstate, ExplainState *es)
if (es->format == EXPLAIN_FORMAT_TEXT)
{
ExplainIndentText(es);
- appendStringInfo(es->str, "Sort Method: %s %s: %ldkB\n",
+ appendStringInfo(es->str, "Sort Method: %s %s: "INT64_FORMAT "kB\n",
sortMethod, spaceType, spaceUsed);
}
else
@@ -2715,7 +2715,7 @@ show_sort_info(SortState *sortstate, ExplainState *es)
TuplesortInstrumentation *sinstrument;
const char *sortMethod;
const char *spaceType;
- long spaceUsed;
+ int64 spaceUsed;
sinstrument = &sortstate->shared_info->sinstrument[n];
if (sinstrument->sortMethod == SORT_TYPE_STILL_IN_PROGRESS)
@@ -2731,7 +2731,7 @@ show_sort_info(SortState *sortstate, ExplainState *es)
{
ExplainIndentText(es);
appendStringInfo(es->str,
- "Sort Method: %s %s: %ldkB\n",
+ "Sort Method: %s %s: "INT64_FORMAT "kB\n",
sortMethod, spaceType, spaceUsed);
}
else
@@ -2795,23 +2795,23 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
if (groupInfo->maxMemorySpaceUsed > 0)
{
- long avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount;
+ int64 avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount;
const char *spaceTypeName;
spaceTypeName = tuplesort_space_type_name(SORT_SPACE_TYPE_MEMORY);
- appendStringInfo(es->str, " Average %s: %ldkB Peak %s: %ldkB",
+ appendStringInfo(es->str, " Average %s: " INT64_FORMAT "kB Peak %s: " INT64_FORMAT "kB",
spaceTypeName, avgSpace,
spaceTypeName, groupInfo->maxMemorySpaceUsed);
}
if (groupInfo->maxDiskSpaceUsed > 0)
{
- long avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
+ int64 avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
const char *spaceTypeName;
spaceTypeName = tuplesort_space_type_name(SORT_SPACE_TYPE_DISK);
- appendStringInfo(es->str, " Average %s: %ldkB Peak %s: %ldkB",
+ appendStringInfo(es->str, " Average %s: " INT64_FORMAT "kB Peak %s: " INT64_FORMAT "kB",
spaceTypeName, avgSpace,
spaceTypeName, groupInfo->maxDiskSpaceUsed);
}
@@ -2829,7 +2829,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
if (groupInfo->maxMemorySpaceUsed > 0)
{
- long avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount;
+ int64 avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount;
const char *spaceTypeName;
StringInfoData memoryName;
@@ -2846,7 +2846,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
}
if (groupInfo->maxDiskSpaceUsed > 0)
{
- long avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
+ int64 avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
const char *spaceTypeName;
StringInfoData diskName;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index f5dfa32d55..ccce8f20f3 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2032,10 +2032,10 @@ typedef struct SortState
typedef struct IncrementalSortGroupInfo
{
int64 groupCount;
- long maxDiskSpaceUsed;
- long totalDiskSpaceUsed;
- long maxMemorySpaceUsed;
- long totalMemorySpaceUsed;
+ int64 maxDiskSpaceUsed;
+ int64 totalDiskSpaceUsed;
+ int64 maxMemorySpaceUsed;
+ int64 totalMemorySpaceUsed;
bits32 sortMethods; /* bitmask of TuplesortMethod */
} IncrementalSortGroupInfo;
diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h
index d992b4875a..9e76666fe9 100644
--- a/src/include/utils/tuplesort.h
+++ b/src/include/utils/tuplesort.h
@@ -90,7 +90,7 @@ typedef struct TuplesortInstrumentation
{
TuplesortMethod sortMethod; /* sort algorithm used */
TuplesortSpaceType spaceType; /* type of space spaceUsed represents */
- long spaceUsed; /* space consumption, in kB */
+ int64 spaceUsed; /* space consumption, in kB */
} TuplesortInstrumentation;
--
2.20.1
On Thu, Jul 2, 2020 at 12:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
That's from POSIX, though. I imagine MSVC won't be happy (surprise!).
We've got quite a few uses of it already, so apparently it's fine.
Oh, looks like we have a compatibility hack for MSVC within
win32_port.h, where ssize_t is typedef'd to __int64. I didn't realize
that it was okay to use ssize_t.
--
Peter Geoghegan
On Thu, Jul 2, 2020 at 12:47 PM James Coleman <jtc331@gmail.com> wrote:
But wouldn't that mean we'd get int on 32-bit systems, and since we're
accumulating data we could go over that value in both memory and disk?My assumption is that it's preferable to have the "this run value" and
the "total used across multiple runs" and both of those for disk and
memory to be the same. In that case it seems we want to guarantee
64-bits.
I agree. There seems to be little reason to accommodate platform level
conventions, beyond making sure that everything works on less popular
or obsolete platforms.
I suppose that it's a little idiosyncratic to use int64 like this. But
it makes sense, and isn't nearly as ugly as the long thing, so I don't
think that it should really matter.
--
Peter Geoghegan
James Coleman <jtc331@gmail.com> writes:
On Thu, Jul 2, 2020 at 3:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
mumble ssize_t mumble
But wouldn't that mean we'd get int on 32-bit systems, and since we're
accumulating data we could go over that value in both memory and disk?
Certainly, a number that's meant to represent the amount of data *on disk*
shouldn't use ssize_t. But I think it's appropriate if you want to
represent in-memory quantities while also allowing negative values.
I guess if you're expecting in-memory sizes exceeding 2GB, you might worry
that ssize_t could overflow. I'm dubious that a 32-bit machine could get
to that, though, seeing that it's going to have other demands on its
address space.
My assumption is that it's preferable to have the "this run value" and
the "total used across multiple runs" and both of those for disk and
memory to be the same. In that case it seems we want to guarantee
64-bits.
If you're not going to distinguish in-memory from not-in-memory, agreed.
regards, tom lane
On Fri, 3 Jul 2020 at 07:47, James Coleman <jtc331@gmail.com> wrote:
Patch using int64 attached.
I added this to the open items list for PG13.
David
On Thu, Jul 30, 2020 at 10:12 PM David Rowley <dgrowleyml@gmail.com> wrote:
On Fri, 3 Jul 2020 at 07:47, James Coleman <jtc331@gmail.com> wrote:
Patch using int64 attached.
I added this to the open items list for PG13.
David
I'd previously attached a patch [1]/messages/by-id/CAAaqYe_Y5zwCTFCJeso7p34yJgf4khR8EaKeJtGd=QPudOad6A@mail.gmail.com, and there seemed to be agreement
it was reasonable (lightly so, but I also didn't see any
disagreement); would someone be able to either commit the change or
provide some additional feedback?
Thanks,
James
[1]: /messages/by-id/CAAaqYe_Y5zwCTFCJeso7p34yJgf4khR8EaKeJtGd=QPudOad6A@mail.gmail.com
On Sat, 1 Aug 2020 at 02:02, James Coleman <jtc331@gmail.com> wrote:
I'd previously attached a patch [1], and there seemed to be agreement
it was reasonable (lightly so, but I also didn't see any
disagreement); would someone be able to either commit the change or
provide some additional feedback?
It looks fine to me. Pushed.
David
Show quoted text
[1]: /messages/by-id/CAAaqYe_Y5zwCTFCJeso7p34yJgf4khR8EaKeJtGd=QPudOad6A@mail.gmail.com