From c7654f6a37792ab9525ff98b710c23b27c7868a5 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 6 Apr 2020 22:22:13 -0400 Subject: [PATCH] Fix representation of SORT_TYPE_STILL_IN_PROGRESS. It turns out that the code did indeed rely on a zeroed TuplesortInstrumentation.sortMethod field to indicate "this worker never did anything", although it seems the issue only comes up during certain race-condition-y cases. Hence, rearrange the TuplesortMethod enum to restore SORT_TYPE_STILL_IN_PROGRESS to having the value zero, and add some comments reinforcing that that isn't optional. Also future-proof a loop over the possible values of the enum. sizeof(bits32) happened to be the correct limit value, but only by purest coincidence. Per buildfarm and local investigation. Discussion: https://postgr.es/m/12222.1586223974@sss.pgh.pa.us --- src/backend/commands/explain.c | 12 ++++++------ src/include/utils/tuplesort.h | 20 +++++++++++++------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index cad10662bb0..baaa5817af7 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -2762,14 +2762,14 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo, List *methodNames = NIL; /* Generate a list of sort methods used across all groups. */ - for (int bit = 0; bit < sizeof(bits32); ++bit) + for (int bit = 0; bit < NUM_TUPLESORTMETHODS; bit++) { - if (groupInfo->sortMethods & (1 << bit)) - { - TuplesortMethod sortMethod = (1 << bit); - const char *methodName; + TuplesortMethod sortMethod = (1 << bit); + + if (groupInfo->sortMethods & sortMethod) + { + const char *methodName = tuplesort_method_name(sortMethod); - methodName = tuplesort_method_name(sortMethod); methodNames = lappend(methodNames, unconstify(char *, methodName)); } } diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h index 8d00a9e5016..04d263228d4 100644 --- a/src/include/utils/tuplesort.h +++ b/src/include/utils/tuplesort.h @@ -62,18 +62,24 @@ typedef struct SortCoordinateData *SortCoordinate; * TuplesortInstrumentation can't contain any pointers because we * sometimes put it in shared memory. * - * TuplesortMethod is used in a bitmask in Increment Sort's shared memory - * instrumentation so needs to have each value be a separate bit. + * The parallel-sort infrastructure relies on having a zero TuplesortMethod + * indicate that a worker never did anything, so we assign zero to + * SORT_TYPE_STILL_IN_PROGRESS. The other values of this enum can be + * OR'ed together to represent a situation where different workers used + * different methods, so we need a separate bit for each one. Keep the + * NUM_TUPLESORTMETHODS constant in sync with the number of bits! */ typedef enum { - SORT_TYPE_STILL_IN_PROGRESS = 1 << 0, - SORT_TYPE_TOP_N_HEAPSORT = 1 << 1, - SORT_TYPE_QUICKSORT = 1 << 2, - SORT_TYPE_EXTERNAL_SORT = 1 << 3, - SORT_TYPE_EXTERNAL_MERGE = 1 << 4 + SORT_TYPE_STILL_IN_PROGRESS = 0, + SORT_TYPE_TOP_N_HEAPSORT = 1 << 0, + SORT_TYPE_QUICKSORT = 1 << 1, + SORT_TYPE_EXTERNAL_SORT = 1 << 2, + SORT_TYPE_EXTERNAL_MERGE = 1 << 3 } TuplesortMethod; +#define NUM_TUPLESORTMETHODS 4 + typedef enum { SORT_SPACE_TYPE_DISK,