Re: GSoC 2017 : Patch for predicate locking in Gist index

<https://mailtrack.io/> Sent with Mailtrack
<https://chrome.google.com/webstore/detail/mailtrack-for-gmail-inbox/ndnaehgpjlnokgebbaldlmgkapkpjkkb?utm_source=gmail&utm_medium=signature&utm_campaign=signaturevirality>
<#>

On 3 October 2017 at 00:32, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>
wrote:

> On Mon, Oct 2, 2017 at 9:11 PM, Andrew Borodin <amborodin86(at)gmail(dot)com>
> wrote:
>
>> On Mon, Oct 2, 2017 at 8:00 PM, Alexander Korotkov
>> <a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>> > What happen if exactly this "continue" fires?
>> >
>> >> if (GistFollowRight(stack->page))
>> >> {
>> >> if (!xlocked)
>> >> {
>> >> LockBuffer(stack->buffer, GIST_UNLOCK);
>> >> LockBuffer(stack->buffer, GIST_EXCLUSIVE);
>> >> xlocked = true;
>> >> /* someone might've completed the split when we unlocked */
>> >> if (!GistFollowRight(stack->page))
>> >> continue;
>> >
>> >
>> > In this case we might get xlocked == true without calling
>> > CheckForSerializableConflictIn().
>> Indeed! I've overlooked it. I'm remembering this issue, we were
>> considering not fixing splits if in Serializable isolation, but
>> dropped the idea.
>>
>
> Yeah, current insert algorithm assumes that split must be fixed before we
> can correctly traverse the tree downwards.
>
>
>> CheckForSerializableConflictIn() must be after every exclusive lock.
>>
>
> I'm not sure, that fixing split is the case to necessary call
> CheckForSerializableConflictIn(). This lock on leaf page is not taken to
> do modification of the page. It's just taken to ensure that nobody else is
> fixing this split the same this. After fixing the split, it might appear
> that insert would go to another page.
>
> > I think it would be rather safe and easy for understanding to more
>> > CheckForSerializableConflictIn() directly before gistinserttuple().
>> The difference is that after lock we have conditions to change page,
>> and before gistinserttuple() we have actual intent to change page.
>>
>> From the point of future development first version is better (if some
>> new calls occasionally spawn in), but it has 3 calls while your
>> proposal have 2 calls.
>> It seems to me that CheckForSerializableConflictIn() before
>> gistinserttuple() is better as for now.
>>
>
> Agree.
>

I have updated the location of CheckForSerializableConflictIn() and
changed the status of the patch to "needs review".

Regards,
Shubham