Re: WIP: parallel GiST index builds

On 7/30/24 11:39, Andrey M. Borodin wrote:
>
>
>> On 30 Jul 2024, at 14:05, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>>
>>
>>
>> On 7/26/24 14:13, Andrey M. Borodin wrote:
>>>
>>>
>>>> On 26 Jul 2024, at 14:30, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>>>>
>>>> I feel the tricky part about doing that is that we need to make sure the fake LSNs are all less than the current real LSN when the index build completes and while that normally should be the case we will have a almost never exercised code path for when the fake LSN becomes bigger than the real LSN which may contain bugs. Is that really worth it to optimize.
>>>>
>>>> But if we are going to use fake LSN: since the index being built is not visible to any scans we do not have to use GetFakeLSNForUnloggedRel() but could use an own counter in shared memory in the GISTShared struct for this specific index which starts at FirstNormalUnloggedLSN. This would give us slightly less contention plus decrease the risk (for good and bad) of the fake LSN being larger than the real LSN.
>>>
>>> +1 for atomic counter in GISTShared.
>>
>> I tried implementing this, see the attached 0002 patch that replaces the
>> fake LSN with an atomic counter in shared memory. It seems to work (more
>> testing needed), but I can't say I'm very happy with the code :-(
>
> I agree. Passing this pointer everywhere seems ugly.
>

Yeah.

>>
>> The way it passes the shared counter to places that actually need it is
>> pretty ugly. The thing is - the counter needs to be in shared memory,
>> but places like gistplacetopage() have no idea/need of that. I chose to
>> simply pass a pg_atomic_uint64 pointer, but that's ... not pretty. Is
>> there's a better way to do this?
>>
>> I thought maybe we could simply increment the counter before each call
>> and pass the LSN value - 64bits should be enough, not sure about the
>> overhead. But gistplacetopage() also uses the LSN twice, and I'm not
>> sure it'd be legal to use the same value twice.
>>
>> Any better ideas?
>
> How about global pointer to fake LSN?
> Just set it to correct pointer when in parallel build, or NULL either way.
>

I'm not sure adding a global variable is pretty either. What if there's
some error, for example? Will it get reset to NULL?

>>> BTW we can just reset LSNs to GistBuildLSN just before doing log_newpage_range().
>>>
>>
>> Why would the reset be necessary? Doesn't log_newpage_range() set page
>> LSN to current insert LSN? So why would reset that?
>>
>> I'm not sure about the discussion about NSN and the need to handle the
>> case when NSN / fake LSN values get ahead of LSN. Is that really a
>> problem? If the values generated from the counter are private to the
>> index build, and log_newpage_range() replaces them with current LSN, do
>> we still need to worry about it?
>
> Stamping pages with new real LSN will do the trick. I didn’t know that log_newpage_range() is already doing so.
>

I believe it does, or at least that's what I believe this code at the
end is meant to do:

recptr = XLogInsert(RM_XLOG_ID, XLOG_FPI);

for (i = 0; i < nbufs; i++)
{
PageSetLSN(BufferGetPage(bufpack[i]), recptr);
UnlockReleaseBuffer(bufpack[i]);
}

Unless I misunderstood what this does.

> How do we synchronize Shared Fake LSN with global XLogCtl->unloggedLSN? Just bump XLogCtl->unloggedLSN if necessary?
> Perhaps, consider using GetFakeLSNForUnloggedRel() instead of shared counter? As long as we do not care about FakeLSN>RealLSN.
>

I'm confused. How is this related to unloggedLSN at all?

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company