MIME-Version: 1.0
References: <c2ee7d69-ce17-43f2-d1a0-9811edbda6e6@enterprisedb.com>
 <CAEze2WgPH4zdEcULR_N4DwDa3B6NdU--577N0RuP5463HB6K5g@mail.gmail.com>
 <c7339541-6a44-da48-3618-b20816d027f1@enterprisedb.com> <CAEze2Whg43uK9g3CT_qWxWa2PjtcOU_eqTuxjBOOfNuzsPGAMA@mail.gmail.com>
 <2d7c64de-716e-346e-b01e-03db0c2bc5ac@enterprisedb.com> <CAEze2WgJrU61Q12Q4UO0Xcgk1DdTegXss=r3uX1UabiMVT-8gg@mail.gmail.com>
 <2e86059f-4004-eec1-2bc1-9e7fd119c9e2@enterprisedb.com> <CAEze2Wgb+V1AbYueaJ6zMEEBwCMMRsbnQDn-iXV5fXN1JTFQpA@mail.gmail.com>
 <a3506141-d471-7236-7b36-953f4d4a091c@enterprisedb.com> <CAEze2WjugXkONAe9RZSgoVipHX5m6c0LnDZ=FVgWs25Fk-zX2A@mail.gmail.com>
 <fff0b3f5-d5a7-fcef-25bb-ac0427188008@enterprisedb.com> <CAEze2Wg=6w42JiQtGwQvM-nPdgMYNVK-Z85r=_QZ1EPC02E9dQ@mail.gmail.com>
 <5384d42e-1961-c2fd-1955-1a5a99fa1234@enterprisedb.com>
In-Reply-To: <5384d42e-1961-c2fd-1955-1a5a99fa1234@enterprisedb.com>
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Tue, 28 Nov 2023 16:39:33 +0100
Message-ID: <CAEze2WhCSKQDhP3nyARWH5Zf+B-5rtDteA7d4QSDa-BuSSkSPA@mail.gmail.com>
Subject: Re: Parallel CREATE INDEX for BRIN indexes
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAEze2WhCSKQDhP3nyARWH5Zf%2BB-5rtDteA7d4QSDa-BuSSkSPA%40mail.gmail.com>
Precedence: bulk

On Thu, 23 Nov 2023 at 14:35, Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
> On 11/23/23 13:33, Matthias van de Meent wrote:
>> The union operator may leak (lots of) memory, so I think it makes
>> sense to keep a context around that can be reset after we've extracted
>> the merge result.
>>
>
> But does the current code actually achieve that? It does create a "brin
> union" context, but then it only does this:
>
>     /* Use our own memory context to avoid retail pfree */
>     cxt = AllocSetContextCreate(CurrentMemoryContext,
>                                 "brin union",
>                                 ALLOCSET_DEFAULT_SIZES);
>     oldcxt = MemoryContextSwitchTo(cxt);
>     db = brin_deform_tuple(bdesc, b, NULL);
>     MemoryContextSwitchTo(oldcxt);
>
> Surely that does not limit the amount of memory used by the actual union
> functions in any way?

Oh, yes, of course. For some reason I thought that covered the calls
to the union operator function too, but it indeed only covers
deserialization. I do think it is still worthwhile to not do the
create/delete cycle, but won't hold the patch back for that.

>>> However, I don't think the number of union_tuples calls is likely to be
>>> very high, especially for large tables. Because we split the table into
>>> 2048 chunks, and then cap the chunk size by 8192. For large tables
>>> (where this matters) we're likely close to 8192.
>>
>> I agree that the merging part of the index creation is the last part,
>> and usually has no high impact on the total performance of the reindex
>> operation, but in memory-constrained environments releasing and then
>> requesting the same chunk of memory over and over again just isn't
>> great.
>
> OK, I'll take a look at the scratch context you suggested.
>
> My point however was we won't actually do that very often, because on
> large tables the BRIN ranges are likely smaller than the parallel scan
> chunk size, so few overlaps. OTOH if the table is small, or if the BRIN
> ranges are large, there'll be few of them.

That's true, so maybe I'm concerned about something that amounts to
only marginal gains.

I noticed that the v4 patch doesn't yet update the documentation in
indexam.sgml with am->amcanbuildparallel.
Once that is included and reviewed I think this will be ready, unless
you want to address any of my comments upthread (that I marked with
'not in this patch') in this patch.


Kind regards,

Matthias van de Meent
Neon (https://neon.tech)