Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)

Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra <
tomas(dot)vondra(at)enterprisedb(dot)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