Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra < tomas.von...@enterprisedb.com> escreveu:
> > > On 12/26/23 19:10, Ranier Vilela wrote: > > Hi, > > > > The commit b437571 <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. > Thank you for your work. best regards, Ranier Vilela
