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

Em qui., 28 de dez. de 2023 às 22:16, Tomas Vondra <
tomas(dot)vondra(at)enterprisedb(dot)com> escreveu:

>
>
> On 12/27/23 12:37, Ranier Vilela wrote:
> > Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra
> > <tomas(dot)vondra(at)enterprisedb(dot)com <mailto:tomas(dot)vondra(at)enterprisedb(dot)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.
>
With Windows 10, msvc 2022, compile end pass ninja test.

But, if you allow me, I would like to try another approach to
simplification.
Instead of increasing the arguments in the call, wouldn't it be better to
decrease them
and this way all arguments will be passed in the registers instead of on a
stack?

bs_spool may well contain this data and will probably be useful in the
future.

I made a v1 version, based on your patch, for your consideration.

best regards,
Ranier Vilela