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
>From 43545ab6e54821a1d459cde2ff9dea58ece2c237 Mon Sep 17 00:00:00 2001
From: tanghy <tanghy.f...@cn.fujitsu.com>
Date: Fri, 16 Oct 2020 14:50:38 +0900
Subject: [PATCH] Use INT64_FORMAT to print int64 variables in sort debug
Commit 6ee3b5fb99 cleaned up most of the long/int64 confusion related to
incremental sort, but the sort debug messages were still using %ld for
int64 variables. So fix that.
Author: Haiying Tang
Backpatch-through: 13
Discussion:
https://postgr.es/m/4250be9d350c4992abb722a76e288aef%40G08CNEXMBPEKD05.g08.fujitsu.local
---
src/backend/executor/nodeIncrementalSort.c | 28 +++++++++++-----------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/backend/executor/nodeIncrementalSort.c
b/src/backend/executor/nodeIncrementalSort.c
index eb6cc19a60..eb1c1326de 100644
--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -333,7 +333,7 @@ switchToPresortedPrefixMode(PlanState *pstate)
*/
if (node->bounded)
{
- SO1_printf("Setting bound on presorted prefix tuplesort to:
%ld\n",
+ SO1_printf("Setting bound on presorted prefix tuplesort to: "
INT64_FORMAT "\n",
node->bound - node->bound_Done);
tuplesort_set_bound(node->prefixsort_state,
node->bound -
node->bound_Done);
@@ -417,9 +417,9 @@ switchToPresortedPrefixMode(PlanState *pstate)
* remaining in the large single prefix key group we think we've
* encountered.
*/
- SO1_printf("Moving %ld tuples to presorted prefix tuplesort\n",
nTuples);
+ SO1_printf("Moving " INT64_FORMAT " tuples to presorted prefix
tuplesort\n", nTuples);
node->n_fullsort_remaining -= nTuples;
- SO1_printf("Setting n_fullsort_remaining to %ld\n",
node->n_fullsort_remaining);
+ SO1_printf("Setting n_fullsort_remaining to " INT64_FORMAT "\n",
node->n_fullsort_remaining);
if (lastTuple)
{
@@ -449,7 +449,7 @@ switchToPresortedPrefixMode(PlanState *pstate)
* out all of those tuples, and then come back around to find
another
* batch.
*/
- SO1_printf("Sorting presorted prefix tuplesort with %ld
tuples\n", nTuples);
+ SO1_printf("Sorting presorted prefix tuplesort with "
INT64_FORMAT " tuples\n", nTuples);
tuplesort_performsort(node->prefixsort_state);
INSTRUMENT_SORT_GROUP(node, prefixsort);
@@ -462,7 +462,7 @@ switchToPresortedPrefixMode(PlanState *pstate)
* - n), so store the current number of processed
tuples for use
* in configuring sorting bound.
*/
- SO2_printf("Changing bound_Done from %ld to %ld\n",
+ SO2_printf("Changing bound_Done from " INT64_FORMAT "
to " INT64_FORMAT "\n",
Min(node->bound, node->bound_Done +
nTuples), node->bound_Done);
node->bound_Done = Min(node->bound, node->bound_Done +
nTuples);
}
@@ -574,7 +574,7 @@ ExecIncrementalSort(PlanState *pstate)
* need to re-execute the prefix mode transition
function to pull
* out the next prefix key group.
*/
- SO1_printf("Re-calling switchToPresortedPrefixMode()
because n_fullsort_remaining is > 0 (%ld)\n",
+ SO1_printf("Re-calling switchToPresortedPrefixMode()
because n_fullsort_remaining is > 0 (" INT64_FORMAT ")\n",
node->n_fullsort_remaining);
switchToPresortedPrefixMode(pstate);
}
@@ -707,7 +707,7 @@ ExecIncrementalSort(PlanState *pstate)
*/
node->outerNodeDone = true;
- SO1_printf("Sorting fullsort with %ld
tuples\n", nTuples);
+ SO1_printf("Sorting fullsort with "
INT64_FORMAT " tuples\n", nTuples);
tuplesort_performsort(fullsort_state);
INSTRUMENT_SORT_GROUP(node, fullsort);
@@ -776,7 +776,7 @@ ExecIncrementalSort(PlanState *pstate)
* current number of processed
tuples for later use
* configuring the sort state's
bound.
*/
- SO2_printf("Changing bound_Done
from %ld to %ld\n",
+ SO2_printf("Changing bound_Done
from " INT64_FORMAT " to " INT64_FORMAT "\n",
node->bound_Done,
Min(node->bound, node->bound_Done + nTuples));
node->bound_Done =
Min(node->bound, node->bound_Done + nTuples);
@@ -787,7 +787,7 @@ ExecIncrementalSort(PlanState *pstate)
* sort and transition modes to reading
out the sorted
* tuples.
*/
- SO1_printf("Sorting fullsort tuplesort
with %ld tuples\n",
+ SO1_printf("Sorting fullsort tuplesort
with " INT64_FORMAT " tuples\n",
nTuples);
tuplesort_performsort(fullsort_state);
@@ -828,7 +828,7 @@ ExecIncrementalSort(PlanState *pstate)
* on FIFO retrieval semantics when
transferring them to the
* presorted prefix tuplesort.
*/
- SO1_printf("Sorting fullsort tuplesort with %ld
tuples\n", nTuples);
+ SO1_printf("Sorting fullsort tuplesort with "
INT64_FORMAT " tuples\n", nTuples);
tuplesort_performsort(fullsort_state);
INSTRUMENT_SORT_GROUP(node, fullsort);
@@ -847,12 +847,12 @@ ExecIncrementalSort(PlanState *pstate)
{
int64 currentBound =
node->bound - node->bound_Done;
- SO2_printf("Read %ld tuples, but
setting to %ld because we used bounded sort\n",
+ SO2_printf("Read " INT64_FORMAT "
tuples, but setting to " INT64_FORMAT " because we used bounded sort\n",
nTuples,
Min(currentBound, nTuples));
nTuples = Min(currentBound, nTuples);
}
- SO1_printf("Setting n_fullsort_remaining to %ld
and calling switchToPresortedPrefixMode()\n",
+ SO1_printf("Setting n_fullsort_remaining to "
INT64_FORMAT " and calling switchToPresortedPrefixMode()\n",
nTuples);
/*
@@ -942,7 +942,7 @@ ExecIncrementalSort(PlanState *pstate)
* Perform the sort and begin returning the tuples to the
parent plan
* node.
*/
- SO1_printf("Sorting presorted prefix tuplesort with >= %ld
tuples\n", nTuples);
+ SO1_printf("Sorting presorted prefix tuplesort with "
INT64_FORMAT " tuples\n", nTuples);
tuplesort_performsort(node->prefixsort_state);
INSTRUMENT_SORT_GROUP(node, prefixsort);
@@ -958,7 +958,7 @@ ExecIncrementalSort(PlanState *pstate)
* - n), so store the current number of processed
tuples for use
* in configuring sorting bound.
*/
- SO2_printf("Changing bound_Done from %ld to %ld\n",
+ SO2_printf("Changing bound_Done from " INT64_FORMAT "
to " INT64_FORMAT "\n",
node->bound_Done,
Min(node->bound, node->bound_Done +
nTuples));
node->bound_Done = Min(node->bound, node->bound_Done +
nTuples);
--
2.25.4
