> 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.
>
> 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.
>> 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.
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.
Best regards, Andrey Borodin.
