On 2026-Jan-05, Mario González Troncoso wrote: > Hey there. I'm updating the patch.
Thanks. Coincidentally, while I rode the train a few hundred kilometers from your place yesterday, I was editing the comments in 0001 as I had mentioned. The only non-comment change, I think, is that you had left #define NUM_TUPLESORTMETHODS in two places, which was quite odd. I removed the one in tuplesort.h and the comment that accompanied it. Here's what I ended up with. What do you think? I also happened to notice an old typo "its" which should be "it's" in tuplesort.c while reading your patch. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ Y una voz del caos me habló y me dijo "Sonríe y sé feliz, podría ser peor". Y sonreí. Y fui feliz. Y fue peor.
>From 6e11eb1d3786d2c3704eb3ee81e66f46a643dcfa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Mon, 5 Jan 2026 11:26:16 -0300 Subject: [PATCH v3a] edit comments and stuff --- src/backend/utils/sort/tuplesort.c | 2 +- src/include/executor/instrument_node.h | 146 ++++++++++++------------- src/include/utils/tuplesort.h | 15 --- 3 files changed, 73 insertions(+), 90 deletions(-) diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c index 473d31188b8..9e4cd816137 100644 --- a/src/backend/utils/sort/tuplesort.c +++ b/src/backend/utils/sort/tuplesort.c @@ -200,7 +200,7 @@ struct Tuplesortstate int64 maxSpace; /* maximum amount of space occupied among sort * of groups, either in-memory or on-disk */ bool isMaxSpaceDisk; /* true when maxSpace is value for on-disk - * space, false when its value for in-memory + * space, false when it's value for in-memory * space */ TupSortStatus maxSpaceStatus; /* sort status when maxSpace was reached */ LogicalTapeSet *tapeset; /* logtape.c object for tapes in a temp file */ diff --git a/src/include/executor/instrument_node.h b/src/include/executor/instrument_node.h index 75520008c36..49627eb4a27 100644 --- a/src/include/executor/instrument_node.h +++ b/src/include/executor/instrument_node.h @@ -1,8 +1,12 @@ /*------------------------------------------------------------------------- * * instrument_node.h - * Definitions for node-specific instrumentation + * Definitions for node-specific support for parallel query instrumentation * + * These structs purposely contain no pointers because they are copied + * across processes during parallel query execution. Each worker copies its + * individual information into the container struct at executor shutdown time, + * to allow the leader to display the information in EXPLAIN ANALYZE. * * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California @@ -15,32 +19,30 @@ #define INSTRUMENT_NODE_H - /* --------------------- - * per-worker aggregate information + * Instrumentation information for aggregate function execution * --------------------- */ typedef struct AggregateInstrumentation { - Size hash_mem_peak; /* peak hash table memory usage */ - uint64 hash_disk_used; /*kB of disk space used */ - int hash_batches_used; /* batches used during entire execution */ + Size hash_mem_peak; /* peak hash table memory usage */ + uint64 hash_disk_used; /* kB of disk space used */ + int hash_batches_used; /* batches used during entire execution */ } AggregateInstrumentation; -/* ---------------- - * Shared memory container for per-worker aggregate information - * ---------------- +/* + * Shared memory container for per-worker aggregate information */ typedef struct SharedAggInfo { - int num_workers; + int num_workers; AggregateInstrumentation sinstrument[FLEXIBLE_ARRAY_MEMBER]; } SharedAggInfo; -/* - * Struct for statistics maintained by amgettuple and amgetbitmap - * - * Note: IndexScanInstrumentation can't contain any pointers, since it is - * copied into a SharedIndexScanInstrumentation during parallel scans + + +/* --------------------- + * Instrumentation information for indexscans (amgettuple and amgetbitmap) + * --------------------- */ typedef struct IndexScanInstrumentation { @@ -49,7 +51,7 @@ typedef struct IndexScanInstrumentation } IndexScanInstrumentation; /* - * Struct for every worker's IndexScanInstrumentation, stored in shared memory + * Shared memory container for per-worker information */ typedef struct SharedIndexScanInstrumentation { @@ -57,11 +59,13 @@ typedef struct SharedIndexScanInstrumentation IndexScanInstrumentation winstrument[FLEXIBLE_ARRAY_MEMBER]; } SharedIndexScanInstrumentation; -/* - * BitmapHeapScanInstrumentation information + +/* --------------------- + * Instrumentation information for bitmap heap scans * * exact_pages total number of exact pages retrieved * lossy_pages total number of lossy pages retrieved + * --------------------- */ typedef struct BitmapHeapScanInstrumentation { @@ -70,11 +74,7 @@ typedef struct BitmapHeapScanInstrumentation } BitmapHeapScanInstrumentation; /* - * Instrumentation data for a parallel bitmap heap scan. - * - * A shared memory struct that each parallel worker copies its - * BitmapHeapScanInstrumentation information into at executor shutdown to - * allow the leader to display the information in EXPLAIN ANALYZE. + * Shared memory container for per-worker information */ typedef struct SharedBitmapHeapInstrumentation { @@ -83,25 +83,27 @@ typedef struct SharedBitmapHeapInstrumentation } SharedBitmapHeapInstrumentation; +/* --------------------- + * Instrumentation information for Memoize + * --------------------- + */ typedef struct MemoizeInstrumentation { - uint64 cache_hits; /* number of rescans where we've found the - * scan parameters values to be cached */ - uint64 cache_misses; /* number of rescans where we've not found the - * scan parameters values to be cached */ - uint64 cache_evictions; /* number of cache entries removed due to - * the need to free memory */ - uint64 cache_overflows; /* number of times we've had to bypass the - * cache when filling it due to not being - * able to free enough space to store the - * current scan's tuples */ - uint64 mem_peak; /* peak memory usage in bytes */ - + uint64 cache_hits; /* number of rescans where we've found the + * scan parameters values to be cached */ + uint64 cache_misses; /* number of rescans where we've not found the + * scan parameters values to be cached */ + uint64 cache_evictions; /* number of cache entries removed due to + * the need to free memory */ + uint64 cache_overflows; /* number of times we've had to bypass the + * cache when filling it due to not being + * able to free enough space to store the + * current scan's tuples */ + uint64 mem_peak; /* peak memory usage in bytes */ } MemoizeInstrumentation; -/* ---------------- - * Shared memory container for per-worker memoize information - * ---------------- +/* + * Shared memory container for per-worker memoize information */ typedef struct SharedMemoizeInfo { @@ -110,27 +112,25 @@ typedef struct SharedMemoizeInfo } SharedMemoizeInfo; -/* - * Data structures for reporting sort statistics. Note that - * TuplesortInstrumentation can't contain any pointers because we - * sometimes put it in shared memory. - * - * The parallel-sort infrastructure relies on having a zero TuplesortMethod - * to 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! +/* --------------------- + * Instrumentation information for Sorts. + * --------------------- */ -#define NUM_TUPLESORTMETHODS 4 - typedef enum { SORT_SPACE_TYPE_DISK, SORT_SPACE_TYPE_MEMORY, } TuplesortSpaceType; +/* + * The parallel-sort infrastructure relies on having a zero TuplesortMethod + * to 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 = 0, @@ -139,40 +139,40 @@ typedef enum SORT_TYPE_EXTERNAL_SORT = 1 << 2, SORT_TYPE_EXTERNAL_MERGE = 1 << 3 } TuplesortMethod; +#define NUM_TUPLESORTMETHODS 4 typedef struct TuplesortInstrumentation { - TuplesortMethod sortMethod; /* sort algorithm used */ + TuplesortMethod sortMethod; /* sort algorithm used */ TuplesortSpaceType spaceType; /* type of space spaceUsed represents */ - int64 spaceUsed; /* space consumption, in kB */ + int64 spaceUsed; /* space consumption, in kB */ } TuplesortInstrumentation; -/* ---------------- - * Shared memory container for per-worker sort information - * ---------------- +/* + * Shared memory container for per-worker sort information */ typedef struct SharedSortInfo { - int num_workers; + int num_workers; TuplesortInstrumentation sinstrument[FLEXIBLE_ARRAY_MEMBER]; } SharedSortInfo; -/* ---------------- - * Values displayed by EXPLAIN ANALYZE - * ---------------- + +/* --------------------- + * Instrumentation information for nodeHash.c + * --------------------- */ typedef struct HashInstrumentation { - int nbuckets; /* number of buckets at end of execution */ - int nbuckets_original; /* planned number of buckets */ - int nbatch; /* number of batches at end of execution */ - int nbatch_original; /* planned number of batches */ - Size space_peak; /* peak memory usage in bytes */ + int nbuckets; /* number of buckets at end of execution */ + int nbuckets_original; /* planned number of buckets */ + int nbatch; /* number of batches at end of execution */ + int nbatch_original; /* planned number of batches */ + Size space_peak; /* peak memory usage in bytes */ } HashInstrumentation; -/* ---------------- - * Shared memory container for per-worker hash information - * ---------------- +/* + * Shared memory container for per-worker information */ typedef struct SharedHashInfo { @@ -180,9 +180,10 @@ typedef struct SharedHashInfo HashInstrumentation hinstrument[FLEXIBLE_ARRAY_MEMBER]; } SharedHashInfo; -/* ---------------- + +/* --------------------- * Instrumentation information for IncrementalSort - * ---------------- + * --------------------- */ typedef struct IncrementalSortGroupInfo { @@ -200,10 +201,7 @@ typedef struct IncrementalSortInfo IncrementalSortGroupInfo prefixsortGroupInfo; } IncrementalSortInfo; -/* ---------------- - * Shared memory container for per-worker incremental sort information - * ---------------- - */ +/* Shared memory container for per-worker incremental sort information */ typedef struct SharedIncrementalSortInfo { int num_workers; diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h index a7e867f36b8..943a2b7dc93 100644 --- a/src/include/utils/tuplesort.h +++ b/src/include/utils/tuplesort.h @@ -62,21 +62,6 @@ typedef struct SortCoordinateData typedef struct SortCoordinateData *SortCoordinate; -/* - * Data structures for reporting sort statistics. Note that - * TuplesortInstrumentation can't contain any pointers because we - * sometimes put it in shared memory. - * - * The parallel-sort infrastructure relies on having a zero TuplesortMethod - * to 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! - */ - -#define NUM_TUPLESORTMETHODS 4 - /* Bitwise option flags for tuple sorts */ #define TUPLESORT_NONE 0 -- 2.47.3
