agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Andy Fan <zhihuifan1213@163.com>
To: Tomas Vondra <tomas@vondra.me>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Parallel CREATE INDEX for GIN indexes
Date: Thu, 29 Aug 2024 12:30:44 +0800
Message-ID: <87ikvkgdcb.fsf@163.com> (raw)
In-Reply-To: <c2753e01-9b06-43f0-a9ae-43638ce4bedf@vondra.me>
References: <6ab4003f-a8b8-4d75-a67f-f25ad98582dc@enterprisedb.com>
<87pltvmgdm.fsf@163.com>
<3b721981-6fa3-4698-a9b6-70b2d8e8fa3b@enterprisedb.com>
<87y18ektdn.fsf@163.com>
<f902c19e-efcf-45b3-9d1e-3669658ec6ff@enterprisedb.com>
<87jzjes2ia.fsf@163.com>
<03abcca0-47b2-4bc1-be05-6c1a3f1c5511@enterprisedb.com>
<74ef5493-c837-4861-afb0-7d07e3a35407@enterprisedb.com>
<6db057fa-3990-4778-9578-aabc20f05db3@enterprisedb.com>
<CAEze2WjB1vpxtvKuWVEThSaB-v4+8H0EXsOB=yLAv8pLcrQuKw@mail.gmail.com>
<CAEze2WiTAeZe4t5wAeRN834xFBqROPmjeK2XTstNko6bbVPX=A@mail.gmail.com>
<531c2afd-6118-4582-8d0a-7bd2ddbde6c5@enterprisedb.com>
<CAEze2WiYtV1DHDi=dS25h+EB3vGX1Q6FoiBxPcU7YjXMKb2_3g@mail.gmail.com>
<3b011125-7489-4ecb-8973-bbe6f00cbf1b@enterprisedb.com>
<CAEze2Wim=EysPVMvdk9WAp0k4+eB5wpef+=N0WBvUC3hVBwrbw@mail.gmail.com>
<148f0f59-55bf-40a9-ab28-51904aa8c325@enterprisedb.com>
<87a5gyqnl5.fsf@163.com>
<c2753e01-9b06-43f0-a9ae-43638ce4bedf@vondra.me>
Tomas Vondra <tomas@vondra.me> writes:
Hi Tomas,
> Yeah. I think we have agreement on 0001-0007.
Yes, the design of 0001-0007 looks good to me and because of the
existing compexitity, I want to foucs on this part for now. I am doing
code review from yesterday, and now my work is done. Just some small
questions:
1. In GinBufferStoreTuple,
/*
* Check if the last TID in the current list is frozen. This is the case
* when merging non-overlapping lists, e.g. in each parallel worker.
*/
if ((buffer->nitems > 0) &&
(ItemPointerCompare(&buffer->items[buffer->nitems - 1], &tup->first) == 0))
buffer->nfrozen = buffer->nitems;
should we do (ItemPointerCompare(&buffer->items[buffer->nitems - 1],
&tup->first) "<=" 0), rather than "=="?
2. Given the "non-overlap" case should be the major case
GinBufferStoreTuple , does it deserve a fastpath for it before calling
ginMergeItemPointers since ginMergeItemPointers have a unconditionally
memory allocation directly, and later we pfree it?
new = ginMergeItemPointers(&buffer->items[buffer->nfrozen], /* first unfronzen */
(buffer->nitems - buffer->nfrozen), /* num of unfrozen */
items, tup->nitems, &nnew);
3. The following comment in index_build is out-of-date now :)
/*
* Determine worker process details for parallel CREATE INDEX. Currently,
* only btree has support for parallel builds.
*
4. Comments - Buffer is not empty and it's storing "a different key"
looks wrong to me. the key may be same and we just need to flush them
because of memory usage. There is the same issue in both
_gin_process_worker_data and _gin_parallel_merge.
if (GinBufferShouldTrim(buffer, tup))
{
Assert(buffer->nfrozen > 0);
state->buildStats.nTrims++;
/*
* Buffer is not empty and it's storing a different key - flush
* the data into the insert, and start a new entry for current
* GinTuple.
*/
AssertCheckItemPointers(buffer, true);
I also run valgrind testing with some testcase, no memory issue is
found.
> I'm a bit torn about 0008, I have not expected changing tuplesort like
> this when I started working
> on the patch, but I can't deny it's a massive speedup for some cases
> (where the patch doesn't help otherwise). But then in other cases it
> doesn't help at all, and 0010 helps.
Yes, I'd like to see these improvements both 0008 and 0010 as a
dedicated improvement.
--
Best Regards
Andy Fan
view thread (78+ messages) latest in thread
Message-ID: <87ikvkgdcb.fsf@163.com>
Permalink: ../87ikvkgdcb.fsf@163.com/
Also on: postgresql.org/message-id/87ikvkgdcb.fsf@163.com
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: zhihuifan1213@163.com, tomas@vondra.me, tomas.vondra@enterprisedb.com, boekewurm+postgres@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Parallel CREATE INDEX for GIN indexes
In-Reply-To: <87ikvkgdcb.fsf@163.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox