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

On 9 October 2017 at 18:57, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>
wrote:

> On Thu, Oct 5, 2017 at 9:48 PM, Shubham Barai <shubhambaraiss(at)gmail(dot)com>
> wrote:
>
>> 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".
>>
>
> Now, ITSM that predicate locks and conflict checks are placed right for
> now.
> However, it would be good to add couple comments to gistdoinsert() whose
> would state why do we call CheckForSerializableConflictIn() in these
> particular places.
>
> I also take a look at isolation tests. You made two separate test specs:
> one to verify that serialization failures do fire, and another to check
> there are no false positives.
> I wonder if we could merge this two test specs into one, but use more
> variety of statements with different keys for both inserts and selects.
>

Please find the updated version of patch here. I have made suggested
changes.

Regards,
Shubham