On 12/27/23 12:37, Ranier Vilela wrote:
> Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra
> <tomas.von...@enterprisedb.com <mailto:tomas.von...@enterprisedb.com>>
> escreveu:
>
>
>
> On 12/26/23 19:10, Ranier Vilela wrote:
> > Hi,
> >
> > The commit b437571
> <http://b437571714707bc6466abde1a0af5e69aaade09c
> <http://b437571714707bc6466abde1a0af5e69aaade09c>> I
> > think has an oversight.
> > When allocate memory and initialize private spool in function:
> > _brin_leader_participate_as_worker
> >
> > The behavior is the bs_spool (heap and index fields)
> > are left empty.
> >
> > The code affected is:
> > buildstate->bs_spool = (BrinSpool *) palloc0(sizeof(BrinSpool));
> > - buildstate->bs_spool->heap = buildstate->bs_spool->heap;
> > - buildstate->bs_spool->index = buildstate->bs_spool->index;
> > + buildstate->bs_spool->heap = heap;
> > + buildstate->bs_spool->index = index;
> >
> > Is the fix correct?
> >
>
> Thanks for noticing this.
>
> You're welcome.
>
>
> Yes, I believe this is a bug - the assignments
> are certainly wrong, it leaves the fields set to NULL.
>
> I wonder how come this didn't fail during testing. Surely, if the leader
> participates as a worker, the tuplesort_begin_index_brin shall be called
> with heap/index being NULL, leading to some failure during the sort. But
> maybe this means we don't actually need the heap/index fields, it's just
> a copy of TuplesortIndexArg, but BRIN does not need that because we sort
> the tuples by blkno, and we don't need the descriptors for that.
>
> Unfortunately I can't test on Windows, since I can't build with meson on
> Windows.
>
>
> In any case, the _brin_parallel_scan_and_build does not actually need
> the separate heap/index arguments, those are already in the spool.
>
> Yeah, for sure.
>
>
> I'll try to figure out if we want to simplify the tuplesort or remove
> the arguments from _brin_parallel_scan_and_build.
>
Here is a patch simplifying the BRIN parallel create code a little bit.
As I suspected, we don't need the heap/index in the spool at all, and we
don't need to pass it to tuplesort_begin_index_brin either - we only
need blkno, and we have that in the datum1 field. This also means we
don't need TuplesortIndexBrinArg.
I'll push this tomorrow, probably.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 23f081389b2..a58f662f2cf 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -56,8 +56,6 @@
typedef struct BrinSpool
{
Tuplesortstate *sortstate; /* state data for tuplesort.c */
- Relation heap;
- Relation index;
} BrinSpool;
/*
@@ -1144,8 +1142,6 @@ brinbuild(Relation heap, Relation index, IndexInfo *indexInfo)
RelationGetNumberOfBlocks(heap));
state->bs_spool = (BrinSpool *) palloc0(sizeof(BrinSpool));
- state->bs_spool->heap = heap;
- state->bs_spool->index = index;
/*
* Attempt to launch parallel worker scan when required
@@ -1200,8 +1196,7 @@ brinbuild(Relation heap, Relation index, IndexInfo *indexInfo)
* factor.
*/
state->bs_spool->sortstate =
- tuplesort_begin_index_brin(heap, index,
- maintenance_work_mem, coordinate,
+ tuplesort_begin_index_brin(maintenance_work_mem, coordinate,
TUPLESORT_NONE);
/*
@@ -2706,8 +2701,6 @@ _brin_leader_participate_as_worker(BrinBuildState *buildstate, Relation heap, Re
/* Allocate memory and initialize private spool */
buildstate->bs_spool = (BrinSpool *) palloc0(sizeof(BrinSpool));
- buildstate->bs_spool->heap = buildstate->bs_spool->heap;
- buildstate->bs_spool->index = buildstate->bs_spool->index;
/*
* Might as well use reliable figure when doling out maintenance_work_mem
@@ -2736,8 +2729,8 @@ _brin_leader_participate_as_worker(BrinBuildState *buildstate, Relation heap, Re
static void
_brin_parallel_scan_and_build(BrinBuildState *state, BrinSpool *brinspool,
BrinShared *brinshared, Sharedsort *sharedsort,
- Relation heap, Relation index, int sortmem,
- bool progress)
+ Relation heap, Relation index,
+ int sortmem, bool progress)
{
SortCoordinate coordinate;
TableScanDesc scan;
@@ -2751,9 +2744,7 @@ _brin_parallel_scan_and_build(BrinBuildState *state, BrinSpool *brinspool,
coordinate->sharedsort = sharedsort;
/* Begin "partial" tuplesort */
- brinspool->sortstate = tuplesort_begin_index_brin(brinspool->heap,
- brinspool->index,
- sortmem, coordinate,
+ brinspool->sortstate = tuplesort_begin_index_brin(sortmem, coordinate,
TUPLESORT_NONE);
/* Join parallel scan */
@@ -2763,8 +2754,8 @@ _brin_parallel_scan_and_build(BrinBuildState *state, BrinSpool *brinspool,
scan = table_beginscan_parallel(heap,
ParallelTableScanFromBrinShared(brinshared));
- reltuples = table_index_build_scan(heap, index, indexInfo, true, true,
- brinbuildCallbackParallel, state, scan);
+ reltuples = table_index_build_scan(heap, index, indexInfo,
+ true, true, brinbuildCallbackParallel, state, scan);
/* insert the last item */
form_and_spill_tuple(state);
@@ -2846,8 +2837,6 @@ _brin_parallel_build_main(dsm_segment *seg, shm_toc *toc)
/* Initialize worker's own spool */
buildstate->bs_spool = (BrinSpool *) palloc0(sizeof(BrinSpool));
- buildstate->bs_spool->heap = heapRel;
- buildstate->bs_spool->index = indexRel;
/* Look up shared state private to tuplesort.c */
sharedsort = shm_toc_lookup(toc, PARALLEL_KEY_TUPLESORT, false);
@@ -2865,7 +2854,8 @@ _brin_parallel_build_main(dsm_segment *seg, shm_toc *toc)
_brin_parallel_scan_and_build(buildstate, buildstate->bs_spool,
brinshared, sharedsort,
- heapRel, indexRel, sortmem, false);
+ heapRel, indexRel,
+ sortmem, false);
/* Report WAL/buffer usage during parallel execution */
bufferusage = shm_toc_lookup(toc, PARALLEL_KEY_BUFFER_USAGE, false);
diff --git a/src/backend/utils/sort/tuplesortvariants.c b/src/backend/utils/sort/tuplesortvariants.c
index 90fc605f1ca..27425880a5f 100644
--- a/src/backend/utils/sort/tuplesortvariants.c
+++ b/src/backend/utils/sort/tuplesortvariants.c
@@ -137,16 +137,6 @@ typedef struct
uint32 max_buckets;
} TuplesortIndexHashArg;
-/*
- * Data struture pointed by "TuplesortPublic.arg" for the index_brin subcase.
- */
-typedef struct
-{
- TuplesortIndexArg index;
-
- /* XXX do we need something here? */
-} TuplesortIndexBrinArg;
-
/*
* Data struture pointed by "TuplesortPublic.arg" for the Datum case.
* Set by tuplesort_begin_datum and used only by the DatumTuple routines.
@@ -562,20 +552,13 @@ tuplesort_begin_index_gist(Relation heapRel,
}
Tuplesortstate *
-tuplesort_begin_index_brin(Relation heapRel,
- Relation indexRel,
- int workMem,
+tuplesort_begin_index_brin(int workMem,
SortCoordinate coordinate,
int sortopt)
{
Tuplesortstate *state = tuplesort_begin_common(workMem, coordinate,
sortopt);
TuplesortPublic *base = TuplesortstateGetPublic(state);
- MemoryContext oldcontext;
- TuplesortIndexBrinArg *arg;
-
- oldcontext = MemoryContextSwitchTo(base->maincontext);
- arg = (TuplesortIndexBrinArg *) palloc(sizeof(TuplesortIndexBrinArg));
#ifdef TRACE_SORT
if (trace_sort)
@@ -592,12 +575,7 @@ tuplesort_begin_index_brin(Relation heapRel,
base->writetup = writetup_index_brin;
base->readtup = readtup_index_brin;
base->haveDatum1 = true;
- base->arg = arg;
-
- arg->index.heapRel = heapRel;
- arg->index.indexRel = indexRel;
-
- MemoryContextSwitchTo(oldcontext);
+ base->arg = NULL;
return state;
}
diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h
index 357eb35311d..103ced4005f 100644
--- a/src/include/utils/tuplesort.h
+++ b/src/include/utils/tuplesort.h
@@ -430,9 +430,7 @@ extern Tuplesortstate *tuplesort_begin_index_gist(Relation heapRel,
Relation indexRel,
int workMem, SortCoordinate coordinate,
int sortopt);
-extern Tuplesortstate *tuplesort_begin_index_brin(Relation heapRel,
- Relation indexRel,
- int workMem, SortCoordinate coordinate,
+extern Tuplesortstate *tuplesort_begin_index_brin(int workMem, SortCoordinate coordinate,
int sortopt);
extern Tuplesortstate *tuplesort_begin_datum(Oid datumType,
Oid sortOperator, Oid sortCollation,
