From: | Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> |
---|---|
To: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> |
Cc: | Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c) |
Date: | 2023-12-29 17:02:11 |
Message-ID: | CAEudQApjiZGZ-5EsVKGuu8UrvN-iJnpRc-d4TyVwp_uuTwUKeA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Em sex., 29 de dez. de 2023 às 08:53, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
escreveu:
> 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.
>
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
From | Date | Subject | |
---|---|---|---|
Next Message | Jelte Fennema-Nio | 2023-12-29 17:29:06 | Add new protocol message to change GUCs for usage with future protocol-only GUCs |
Previous Message | Corey Huinker | 2023-12-29 16:27:50 | Re: Statistics Import and Export |