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

On 12/27/23 12:37, Ranier Vilela wrote:
> Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra
> <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@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