On 7/30/24 11:39, Andrey M. Borodin wrote:
>
>
>> On 30 Jul 2024, at 14:05, Tomas Vondra <tomas.von...@enterprisedb.com> wrote:
>>
>>
>>
>> On 7/26/24 14:13, Andrey M. Borodin wrote:
>>>
>>>
>>>> On 26 Jul 2024, at 14:30, Andreas Karlsson <andr...@proxel.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
