From 625880de662181fd433eb583a00af0f16d57c420 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Wed, 22 Nov 2023 19:58:10 +0100 Subject: [PATCH v4 2/2] review fixes --- contrib/bloom/blutils.c | 1 + src/backend/access/brin/brin.c | 28 ++++++++----------- src/backend/access/gin/ginutil.c | 1 + src/backend/access/gist/gist.c | 1 + src/backend/access/hash/hash.c | 1 + src/backend/access/nbtree/nbtree.c | 1 + src/backend/access/spgist/spgutils.c | 1 + src/backend/catalog/index.c | 3 +- src/include/access/amapi.h | 2 ++ .../modules/dummy_index_am/dummy_index_am.c | 1 + 10 files changed, 22 insertions(+), 18 deletions(-) diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c index f23fbb1d9e0..7451fb1b3bb 100644 --- a/contrib/bloom/blutils.c +++ b/contrib/bloom/blutils.c @@ -122,6 +122,7 @@ blhandler(PG_FUNCTION_ARGS) amroutine->amclusterable = false; amroutine->ampredlocks = false; amroutine->amcanparallel = false; + amroutine->amcanbuildparallel = false; amroutine->amcaninclude = false; amroutine->amusemaintenanceworkmem = false; amroutine->amparallelvacuumoptions = diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 0d3d728c9bf..e836cbaad0b 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -44,11 +44,11 @@ #include "utils/tuplesort.h" /* Magic numbers for parallel state sharing */ -#define PARALLEL_KEY_BRIN_SHARED UINT64CONST(0xA000000000000001) -#define PARALLEL_KEY_TUPLESORT UINT64CONST(0xA000000000000002) -#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xA000000000000003) -#define PARALLEL_KEY_WAL_USAGE UINT64CONST(0xA000000000000004) -#define PARALLEL_KEY_BUFFER_USAGE UINT64CONST(0xA000000000000005) +#define PARALLEL_KEY_BRIN_SHARED UINT64CONST(0xB000000000000001) +#define PARALLEL_KEY_TUPLESORT UINT64CONST(0xB000000000000002) +#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xB000000000000003) +#define PARALLEL_KEY_WAL_USAGE UINT64CONST(0xB000000000000004) +#define PARALLEL_KEY_BUFFER_USAGE UINT64CONST(0xB000000000000005) /* * Status record for spooling/sorting phase. @@ -161,7 +161,7 @@ typedef struct BrinLeader typedef struct BrinBuildState { Relation bs_irel; - int bs_numtuples; + double bs_numtuples; double bs_reltuples; Buffer bs_currentInsertBuf; BlockNumber bs_pagesPerRange; @@ -244,6 +244,7 @@ brinhandler(PG_FUNCTION_ARGS) amroutine->amclusterable = false; amroutine->ampredlocks = false; amroutine->amcanparallel = false; + amroutine->amcanbuildparallel = true; amroutine->amcaninclude = false; amroutine->amusemaintenanceworkmem = false; amroutine->amsummarizing = true; @@ -960,7 +961,7 @@ brinbuildCallback(Relation index, /* * A version of the callback, used by parallel index builds. The main difference * is that instead of writing the BRIN tuples into the index, we write them into - * a shared tuplestore, and leave the insertion up to the leader (which may + * a shared tuplesort, and leave the insertion up to the leader (which may * reorder them a bit etc.). The callback also does not generate empty ranges, * those may be added by the leader when merging results from workers. */ @@ -1880,7 +1881,7 @@ form_and_insert_tuple(BrinBuildState *state) /* * Given a deformed tuple in the build state, convert it into the on-disk - * format and write it to a (shared) tuplestore (the leader will insert it + * format and write it to a (shared) tuplesort (the leader will insert it * into the index later). */ static void @@ -2419,7 +2420,7 @@ _brin_end_parallel(BrinLeader *brinleader, BrinBuildState *state) BlockNumber prevblkno = InvalidBlockNumber; BrinTuple *emptyTuple = NULL; Size emptySize; - BrinSpool *spool = state->bs_spool; + BrinSpool *spool; /* Shutdown worker processes */ WaitForParallelWorkersToFinish(brinleader->pcxt); @@ -2431,6 +2432,8 @@ _brin_end_parallel(BrinLeader *brinleader, BrinBuildState *state) state->bs_reltuples = brinshared->reltuples; state->bs_numtuples = brinshared->indtuples; + /* do the actual sort in the leader */ + spool = state->bs_spool; tuplesort_performsort(spool->sortstate); /* @@ -2443,13 +2446,6 @@ _brin_end_parallel(BrinLeader *brinleader, BrinBuildState *state) * Read the BRIN tuples from the shared tuplesort, sorted by block number. * That probably gives us an index that is cheaper to scan, thanks to mostly * getting data from the same index page as before. - * - * FIXME This probably needs some memory management fixes - we're reading - * tuples from the tuplesort, we're allocating an empty tuple, and so on. - * Probably better to release this memory. - * - * XXX We can't quite free the BrinTuple, though, because that's a field - * in BrinSortTuple. */ while ((btup = tuplesort_getbrintuple(spool->sortstate, &tuplen, true)) != NULL) { diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c index 7a4cd93f301..d4c9d678223 100644 --- a/src/backend/access/gin/ginutil.c +++ b/src/backend/access/gin/ginutil.c @@ -54,6 +54,7 @@ ginhandler(PG_FUNCTION_ARGS) amroutine->amclusterable = false; amroutine->ampredlocks = true; amroutine->amcanparallel = false; + amroutine->amcanbuildparallel = false; amroutine->amcaninclude = false; amroutine->amusemaintenanceworkmem = true; amroutine->amsummarizing = false; diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c index 8ef5fa03290..acec490912d 100644 --- a/src/backend/access/gist/gist.c +++ b/src/backend/access/gist/gist.c @@ -76,6 +76,7 @@ gisthandler(PG_FUNCTION_ARGS) amroutine->amclusterable = true; amroutine->ampredlocks = true; amroutine->amcanparallel = false; + amroutine->amcanbuildparallel = false; amroutine->amcaninclude = true; amroutine->amusemaintenanceworkmem = false; amroutine->amsummarizing = false; diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c index 7a025f33cfe..74592c7d428 100644 --- a/src/backend/access/hash/hash.c +++ b/src/backend/access/hash/hash.c @@ -73,6 +73,7 @@ hashhandler(PG_FUNCTION_ARGS) amroutine->amclusterable = false; amroutine->ampredlocks = true; amroutine->amcanparallel = false; + amroutine->amcanbuildparallel = false; amroutine->amcaninclude = false; amroutine->amusemaintenanceworkmem = false; amroutine->amsummarizing = false; diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c index a88b36a589a..2aba6f5b91e 100644 --- a/src/backend/access/nbtree/nbtree.c +++ b/src/backend/access/nbtree/nbtree.c @@ -112,6 +112,7 @@ bthandler(PG_FUNCTION_ARGS) amroutine->amclusterable = true; amroutine->ampredlocks = true; amroutine->amcanparallel = true; + amroutine->amcanbuildparallel = true; amroutine->amcaninclude = true; amroutine->amusemaintenanceworkmem = false; amroutine->amsummarizing = false; diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c index c112e1e5dd4..77b6af694fb 100644 --- a/src/backend/access/spgist/spgutils.c +++ b/src/backend/access/spgist/spgutils.c @@ -60,6 +60,7 @@ spghandler(PG_FUNCTION_ARGS) amroutine->amclusterable = false; amroutine->ampredlocks = false; amroutine->amcanparallel = false; + amroutine->amcanbuildparallel = false; amroutine->amcaninclude = true; amroutine->amusemaintenanceworkmem = false; amroutine->amsummarizing = false; diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 37e4305d50a..40abbaf476b 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2982,8 +2982,7 @@ index_build(Relation heapRelation, * Note that planner considers parallel safety for us. */ if (parallel && IsNormalProcessingMode() && - (indexRelation->rd_rel->relam == BTREE_AM_OID || - indexRelation->rd_rel->relam == BRIN_AM_OID)) + indexRelation->rd_indam->amcanbuildparallel) indexInfo->ii_ParallelWorkers = plan_create_index_workers(RelationGetRelid(heapRelation), RelationGetRelid(indexRelation)); diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h index 995725502a6..408bb7595bb 100644 --- a/src/include/access/amapi.h +++ b/src/include/access/amapi.h @@ -240,6 +240,8 @@ typedef struct IndexAmRoutine bool ampredlocks; /* does AM support parallel scan? */ bool amcanparallel; + /* does AM support parallel build? */ + bool amcanbuildparallel; /* does AM support columns included with clause INCLUDE? */ bool amcaninclude; /* does AM use maintenance_work_mem? */ diff --git a/src/test/modules/dummy_index_am/dummy_index_am.c b/src/test/modules/dummy_index_am/dummy_index_am.c index cbdae7ab7a5..eaa0c483b7e 100644 --- a/src/test/modules/dummy_index_am/dummy_index_am.c +++ b/src/test/modules/dummy_index_am/dummy_index_am.c @@ -294,6 +294,7 @@ dihandler(PG_FUNCTION_ARGS) amroutine->amclusterable = false; amroutine->ampredlocks = false; amroutine->amcanparallel = false; + amroutine->amcanbuildparallel = false; amroutine->amcaninclude = false; amroutine->amusemaintenanceworkmem = false; amroutine->amsummarizing = false; -- 2.41.0