From: Andy Fan <zhihuifan1213@163.com>
To: Tomas Vondra <tomas@vondra.me>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>,  Matthias van de Meent
 <boekewurm+postgres@gmail.com>,  PostgreSQL Hackers
 <pgsql-hackers@lists.postgresql.org>
Subject: Re: Parallel CREATE INDEX for GIN indexes
In-Reply-To: <c2753e01-9b06-43f0-a9ae-43638ce4bedf@vondra.me> (Tomas Vondra's
	message of "Tue, 27 Aug 2024 13:16:25 +0200")
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>
Date: Thu, 29 Aug 2024 12:30:44 +0800
Message-ID: <87ikvkgdcb.fsf@163.com>
MIME-Version: 1.0
Content-Type: text/plain
Archived-At: <https://www.postgresql.org/message-id/87ikvkgdcb.fsf%40163.com>
Precedence: bulk

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