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

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index cad10662bb086732ba9fa073fb1f8ea2dbd22801..baaa5817af727731851185f5c0de7dd08f1fa52a 100644 (file)
--- 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);
+
+       if (groupInfo->sortMethods & sortMethod)
        {
-           TuplesortMethod sortMethod = (1 << bit);
-           const char *methodName;
+           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 8d00a9e50169133afa4ae3ebbb5cb82a7ecbffe2..04d263228d4ead529fe6860ebd870c319a06c0b3 100644 (file)
--- 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,