Em sex., 29 de dez. de 2023 às 08:53, Ranier Vilela <ranier...@gmail.com> escreveu:
> Em qui., 28 de dez. de 2023 às 22:16, Tomas Vondra < > tomas.von...@enterprisedb.com> escreveu: > >> >> >> 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. >> > 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. > As I wrote, the new patch version was for consideration. It seems more like a question of style, so it's better to remove it. Anyway +1 for your original patch. Best regards, Ranier Vilela
