Horton

Thread

Commits

  1. Add predicate locking for GiST

  1. GSoC 2017 : Patch for predicate locking in Gist index

    Shubham Barai <shubhambaraiss@gmail.com> — 2017-06-16T10:24:13Z

    Hi, hackers!

I have created my first patch for predicate locking in gist index. It
includes a test for verification of serialization failures and a test to
check false positives.
I am submitting my patch little late because there were some issues with
"make check" that I was trying to solve. Now, the patch passes all existing
tests.

Regards,
Shubham



  <https://mailtrack.io/> Sent with Mailtrack
<https://mailtrack.io/install?source=signature&lang=en&referral=shubhambaraiss@gmail.com&idSignature=22>

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

    Shubham Barai <shubhambaraiss@gmail.com> — 2017-06-18T11:48:23Z

    Hi,

Please find the updated patch here.

Regards,
Shubham

On 16 June 2017 at 15:54, Shubham Barai <shubhambaraiss@gmail.com> wrote:

> Hi, hackers!
>
> I have created my first patch for predicate locking in gist index. It
> includes a test for verification of serialization failures and a test to
> check false positives.
> I am submitting my patch little late because there were some issues with
> "make check" that I was trying to solve. Now, the patch passes all existing
> tests.
>
> Regards,
> Shubham
>
>
>
>   <https://mailtrack.io/> Sent with Mailtrack
> <https://mailtrack.io/install?source=signature&lang=en&referral=shubhambaraiss@gmail.com&idSignature=22>
>

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-06-21T07:41:36Z

    On 06/16/2017 01:24 PM, Shubham Barai wrote:
> @@ -497,6 +499,13 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
>  			for (ptr = dist->next; ptr; ptr = ptr->next)
>  				UnlockReleaseBuffer(ptr->buffer);
>  		}
> +
> +		for (ptr = dist; ptr; ptr = ptr->next)
> +			PredicateLockPageSplit(rel,
> +						BufferGetBlockNumber(buffer),
> +						BufferGetBlockNumber(ptr->buffer));
> +
> +

I think this new code needs to go before the UnlockReleaseBuffer() calls 
above. Calling BufferGetBlockNumber() on an already-released buffer is 
not cool.

- Heikki

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-06-21T07:42:46Z

    On 06/21/2017 10:41 AM, Heikki Linnakangas wrote:
> On 06/16/2017 01:24 PM, Shubham Barai wrote:
>> @@ -497,6 +499,13 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
>>  			for (ptr = dist->next; ptr; ptr = ptr->next)
>>  				UnlockReleaseBuffer(ptr->buffer);
>>  		}
>> +
>> +		for (ptr = dist; ptr; ptr = ptr->next)
>> +			PredicateLockPageSplit(rel,
>> +						BufferGetBlockNumber(buffer),
>> +						BufferGetBlockNumber(ptr->buffer));
>> +
>> +
>
> I think this new code needs to go before the UnlockReleaseBuffer() calls
> above. Calling BufferGetBlockNumber() on an already-released buffer is
> not cool.

.. and that's exactly what you fixed in your updated patch. Sorry for 
the noise :-)

- Heikki

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

    Shubham Barai <shubhambaraiss@gmail.com> — 2017-06-21T07:52:50Z

    Hi,

On 21 June 2017 at 13:11, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> On 06/16/2017 01:24 PM, Shubham Barai wrote:
>
>> @@ -497,6 +499,13 @@ gistplacetopage(Relation rel, Size freespace,
>> GISTSTATE *giststate,
>>                         for (ptr = dist->next; ptr; ptr = ptr->next)
>>                                 UnlockReleaseBuffer(ptr->buffer);
>>                 }
>> +
>> +               for (ptr = dist; ptr; ptr = ptr->next)
>> +                       PredicateLockPageSplit(rel,
>> +
>>  BufferGetBlockNumber(buffer),
>> +
>>  BufferGetBlockNumber(ptr->buffer));
>> +
>> +
>>
>
> I think this new code needs to go before the UnlockReleaseBuffer() calls
> above. Calling BufferGetBlockNumber() on an already-released buffer is not
> cool.
>
> - Heikki
>
> I know that. This is the old version of the patch. I had sent updated
patch later. Please have a look at updated patch.

Regards,
Shubham



  <https://mailtrack.io/> Sent with Mailtrack
<https://mailtrack.io/install?source=signature&lang=en&referral=shubhambaraiss@gmail.com&idSignature=22>

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

    x4mmm@yandex-team.ru — 2017-08-16T09:58:08Z

    Hi, hackers!

> 21 июня 2017 г., в 12:52, Shubham Barai <shubhambaraiss@gmail.com> написал(а):
> 
> Hi,
> ...
> I know that. This is the old version of the patch. I had sent updated patch later. Please have a look at updated patch.
> 
> Regards,
> Shubham


Here is some information for reviewers. This applies to patches for GiST, Hash, GIN and SP-GiST.

As a mentor of the Shubham's GSoC project for every patch regarding predicate locking I have checked:
0. Correctness of implementation (places of predicate lock function calls, but, anyway, this point deserves special attention)
1. Patch applies and installcheck-world passes
2. Patch contains new isolation tests
3. These tests fail if indexes do not implement predicate locking
4. Patch updates documentation

Shubham also had checked that patches work (install check-world) on 1Kb, 8Kb and 32Kb pages.

Best regards, Andrey Borodin, Yandex.

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

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2017-09-28T10:59:51Z

    Hi!

On Wed, Jun 21, 2017 at 10:52 AM, Shubham Barai <shubhambaraiss@gmail.com>
wrote:

> Hi,
>
> On 21 June 2017 at 13:11, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
>> On 06/16/2017 01:24 PM, Shubham Barai wrote:
>>
>>> @@ -497,6 +499,13 @@ gistplacetopage(Relation rel, Size freespace,
>>> GISTSTATE *giststate,
>>>                         for (ptr = dist->next; ptr; ptr = ptr->next)
>>>                                 UnlockReleaseBuffer(ptr->buffer);
>>>                 }
>>> +
>>> +               for (ptr = dist; ptr; ptr = ptr->next)
>>> +                       PredicateLockPageSplit(rel,
>>> +
>>>  BufferGetBlockNumber(buffer),
>>> +
>>>  BufferGetBlockNumber(ptr->buffer));
>>> +
>>> +
>>>
>>
>> I think this new code needs to go before the UnlockReleaseBuffer() calls
>> above. Calling BufferGetBlockNumber() on an already-released buffer is not
>> cool.
>>
>> - Heikki
>>
>> I know that. This is the old version of the patch. I had sent updated
> patch later. Please have a look at updated patch.
>

I took a look on this patch.

In gistdoinsert() you do CheckForSerializableConflictIn() only if page
wasn't exclusively locked before (xlocked is false).

if (!xlocked)
> {
> LockBuffer(stack->buffer, GIST_UNLOCK);
> LockBuffer(stack->buffer, GIST_EXCLUSIVE);
> CheckForSerializableConflictIn(r, NULL, stack->buffer);
> xlocked = true;


However, page might be exclusively locked before.  And in this case
CheckForSerializableConflictIn() would be skipped.  That happens very
rarely (someone fixes incomplete split before we did), but nevertheless.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

    Andrey Borodin <amborodin86@gmail.com> — 2017-10-02T10:40:29Z

    Hi, Alexander!

Thanks for looking into the patch!

On Thu, Sep 28, 2017 at 3:59 PM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:

>
>
> In gistdoinsert() you do CheckForSerializableConflictIn() only if page
> wasn't exclusively locked before (xlocked is false).
>
> if (!xlocked)
>> {
>> LockBuffer(stack->buffer, GIST_UNLOCK);
>> LockBuffer(stack->buffer, GIST_EXCLUSIVE);
>> CheckForSerializableConflictIn(r, NULL, stack->buffer);
>> xlocked = true;
>
>
> However, page might be exclusively locked before.  And in this case
> CheckForSerializableConflictIn() would be skipped.  That happens very
> rarely (someone fixes incomplete split before we did), but nevertheless.
>

if xlocked = true, page was already checked for conflict after setting
exclusive lock on it's buffer.  I still do not see any problem here...

Best regards, Andrey Borodin.

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

    Shubham Barai <shubhambaraiss@gmail.com> — 2017-10-02T13:12:30Z

    On Sep 28, 2017 4:30 PM, "Alexander Korotkov" <a.korotkov@postgrespro.ru>
wrote:

Hi!

On Wed, Jun 21, 2017 at 10:52 AM, Shubham Barai <shubhambaraiss@gmail.com>
wrote:

> Hi,
>
> On 21 June 2017 at 13:11, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
>> On 06/16/2017 01:24 PM, Shubham Barai wrote:
>>
>>> @@ -497,6 +499,13 @@ gistplacetopage(Relation rel, Size freespace,
>>> GISTSTATE *giststate,
>>>                         for (ptr = dist->next; ptr; ptr = ptr->next)
>>>                                 UnlockReleaseBuffer(ptr->buffer);
>>>                 }
>>> +
>>> +               for (ptr = dist; ptr; ptr = ptr->next)
>>> +                       PredicateLockPageSplit(rel,
>>> +
>>>  BufferGetBlockNumber(buffer),
>>> +
>>>  BufferGetBlockNumber(ptr->buffer));
>>> +
>>> +
>>>
>>
>> I think this new code needs to go before the UnlockReleaseBuffer() calls
>> above. Calling BufferGetBlockNumber() on an already-released buffer is not
>> cool.
>>
>> - Heikki
>>
>> I know that. This is the old version of the patch. I had sent updated
> patch later. Please have a look at updated patch.
>

I took a look on this patch.

In gistdoinsert() you do CheckForSerializableConflictIn() only if page
wasn't exclusively locked before (xlocked is false).

if (!xlocked)
> {
> LockBuffer(stack->buffer, GIST_UNLOCK);
> LockBuffer(stack->buffer, GIST_EXCLUSIVE);
> CheckForSerializableConflictIn(r, NULL, stack->buffer);
> xlocked = true;


However, page might be exclusively locked before.  And in this case
CheckForSerializableConflictIn() would be skipped.  That happens very
rarely (someone fixes incomplete split before we did), but nevertheless.


I agree with Andrey Borodin's view on locks. I am quoting his message
"if xlocked = true, page was already checked for conflict after setting
exclusive lock on it's buffer.  I still do not see any problem here..."

Regards,
Shubham

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

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2017-10-02T15:00:26Z

    Hi, Andrew!

On Mon, Oct 2, 2017 at 1:40 PM, Andrew Borodin <amborodin86@gmail.com>
wrote:

> Thanks for looking into the patch!
>
> On Thu, Sep 28, 2017 at 3:59 PM, Alexander Korotkov <
> a.korotkov@postgrespro.ru> wrote:
>
>>
>>
>> In gistdoinsert() you do CheckForSerializableConflictIn() only if page
>> wasn't exclusively locked before (xlocked is false).
>>
>> if (!xlocked)
>>> {
>>> LockBuffer(stack->buffer, GIST_UNLOCK);
>>> LockBuffer(stack->buffer, GIST_EXCLUSIVE);
>>> CheckForSerializableConflictIn(r, NULL, stack->buffer);
>>> xlocked = true;
>>
>>
>> However, page might be exclusively locked before.  And in this case
>> CheckForSerializableConflictIn() would be skipped.  That happens very
>> rarely (someone fixes incomplete split before we did), but nevertheless.
>>
>
> if xlocked = true, page was already checked for conflict after setting
> exclusive lock on it's buffer.  I still do not see any problem here...
>

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().  This is very rare codepath, but
still...
I think it would be rather safe and easy for understanding to
more CheckForSerializableConflictIn() directly before gistinserttuple().

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

    Andrey Borodin <amborodin86@gmail.com> — 2017-10-02T18:11:55Z

    On Mon, Oct 2, 2017 at 8:00 PM, Alexander Korotkov
<a.korotkov@postgrespro.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.
CheckForSerializableConflictIn() must be after every exclusive lock.

> 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.

Best regards, Andrey Borodin.

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

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2017-10-02T19:02:26Z

    On Mon, Oct 2, 2017 at 9:11 PM, Andrew Borodin <amborodin86@gmail.com>
wrote:

> On Mon, Oct 2, 2017 at 8:00 PM, Alexander Korotkov
> <a.korotkov@postgrespro.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.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

    Shubham Barai <shubhambaraiss@gmail.com> — 2017-10-05T18:48:13Z

    <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.korotkov@postgrespro.ru>
wrote:

> On Mon, Oct 2, 2017 at 9:11 PM, Andrew Borodin <amborodin86@gmail.com>
> wrote:
>
>> On Mon, Oct 2, 2017 at 8:00 PM, Alexander Korotkov
>> <a.korotkov@postgrespro.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

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

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2017-10-09T13:27:46Z

    On Thu, Oct 5, 2017 at 9:48 PM, Shubham Barai <shubhambaraiss@gmail.com>
wrote:

> On 3 October 2017 at 00:32, Alexander Korotkov <a.korotkov@postgrespro.ru>
> wrote:
>
>> On Mon, Oct 2, 2017 at 9:11 PM, Andrew Borodin <amborodin86@gmail.com>
>> wrote:
>>
>>> On Mon, Oct 2, 2017 at 8:00 PM, Alexander Korotkov
>>> <a.korotkov@postgrespro.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.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

    Shubham Barai <shubhambaraiss@gmail.com> — 2017-10-31T21:10:44Z

    On 9 October 2017 at 18:57, Alexander Korotkov <a.korotkov@postgrespro.ru>
wrote:

> On Thu, Oct 5, 2017 at 9:48 PM, Shubham Barai <shubhambaraiss@gmail.com>
> wrote:
>
>> On 3 October 2017 at 00:32, Alexander Korotkov <a.korotkov@postgrespro.ru
>> > wrote:
>>
>>> On Mon, Oct 2, 2017 at 9:11 PM, Andrew Borodin <amborodin86@gmail.com>
>>> wrote:
>>>
>>>> On Mon, Oct 2, 2017 at 8:00 PM, Alexander Korotkov
>>>> <a.korotkov@postgrespro.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

  16. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2017-11-27T07:47:32Z

    Hi, Shubham!

On Wed, Nov 1, 2017 at 12:10 AM, Shubham Barai <shubhambaraiss@gmail.com>
wrote:

> On 9 October 2017 at 18:57, Alexander Korotkov <a.korotkov@postgrespro.ru>
> wrote:
>
>> 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.
>

In general, patch looks good for me now.  I just see some cosmetic issues.

  /*
> + *Check for any r-w conflicts (in serialisation isolation level)
> + *just before we intend to modify the page
> + */
> + CheckForSerializableConflictIn(r, NULL, stack->buffer);
> + /*


Formatting doesn't look good here.  You've missed space after star sign in
the comment.  You also missed newline after
CheckForSerializableConflictIn() call.

Also, you've long comment lines in predicate-gist.spec.  Please, break long
comments into multiple lines.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  17. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-29T01:53:13Z

    On Mon, Nov 27, 2017 at 4:47 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> Also, you've long comment lines in predicate-gist.spec.  Please, break long
> comments into multiple lines.

Two days is to short to reply. I am moving this patch to next CF.
-- 
Michael

  18. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Shubham Barai <shubhambaraiss@gmail.com> — 2017-11-29T17:50:36Z

    On 27 November 2017 at 13:17, Alexander Korotkov <a.korotkov@postgrespro.ru>
wrote:

> Hi, Shubham!
>
> On Wed, Nov 1, 2017 at 12:10 AM, Shubham Barai <shubhambaraiss@gmail.com>
> wrote:
>
>> On 9 October 2017 at 18:57, Alexander Korotkov <a.korotkov@postgrespro.ru
>> > wrote:
>>
>>> 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.
>>
>
> In general, patch looks good for me now.  I just see some cosmetic issues.
>
>   /*
>> + *Check for any r-w conflicts (in serialisation isolation level)
>> + *just before we intend to modify the page
>> + */
>> + CheckForSerializableConflictIn(r, NULL, stack->buffer);
>> + /*
>
>
> Formatting doesn't look good here.  You've missed space after star sign in
> the comment.  You also missed newline after CheckForSerializableConflictIn()
> call.
>
> Also, you've long comment lines in predicate-gist.spec.  Please, break
> long comments into multiple lines.
>
>
 I have fixed formatting style. Please take a look at updated patch.

Regards,
Shubham

>
>

  19. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    x4mmm@yandex-team.ru — 2018-01-04T16:07:56Z

    Hello everyone!
> 29 нояб. 2017 г., в 22:50, Shubham Barai <shubhambaraiss@gmail.com> написал(а):
> 
>  I have fixed formatting style. Please take a look at updated patch.

Here's rebased patch. Every issue has been addressed, so I'm marking this patch as ready for committer.

Best regards, Andrey Borodin.

  20. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2018-01-04T21:48:23Z

    On Thu, Jan 4, 2018 at 7:07 PM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

> 29 нояб. 2017 г., в 22:50, Shubham Barai <shubhambaraiss@gmail.com>
> написал(а):
>
>  I have fixed formatting style. Please take a look at updated patch.
>
>
> Here's rebased patch. Every issue has been addressed, so I'm marking this
> patch as ready for committer.
>

I'm sorry for concentrating on boring things, but formatting of
predicate-gist.spec still doesn't look good for me.

# To verify serialization failures, queries and permutations are written in
> such
> # a way that an index scan(from one transaction) and an index insert(from
> another
> # transaction) will try to access the same part(sub-tree) of the index.
> #
> # To check reduced false positives, queries and permutations are written
> in such
> # a way that an index scan(from one transaction) and an index insert(from
> another
> # transaction) will try to access different parts(sub-tree) of the index.
>

No space before open bracket (I think it should be when there are multiple
words brackets).
Also, we're trying to fit our lines to 80 characters (if it's not
objectively difficult).
And these are two almost same paragraphs.  I think it should be simplified.

setup
> {
>  create table gist_point_tbl(id int4, p point);
>  create index gist_pointidx on gist_point_tbl using gist(p);
>  insert into gist_point_tbl (id, p)
>  select g, point(g*10, g*10) from generate_series(1, 1000) g;
> }
> setup
> {
>   BEGIN ISOLATION LEVEL SERIALIZABLE;
>   set enable_seqscan=off;
>   set enable_bitmapscan=off;
>   set enable_indexonlyscan=on;
> }
> setup {
>   BEGIN ISOLATION LEVEL SERIALIZABLE;
>   set enable_seqscan=off;
>   set enable_bitmapscan=off;
>   set enable_indexonlyscan=on;
> }


I didn't get idea of using various indentation styles for same purpose.

step "wx3" { insert into gist_point_tbl (id, p)
>                   select g, point(g*500, g*500) from generate_series(12,
> 18) g; }


Indented using spaces here...

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  21. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Shubham Barai <shubhambaraiss@gmail.com> — 2018-01-08T17:14:37Z

    On 5 January 2018 at 03:18, Alexander Korotkov <a.korotkov@postgrespro.ru>
wrote:

> On Thu, Jan 4, 2018 at 7:07 PM, Andrey Borodin <x4mmm@yandex-team.ru>
> wrote:
>
>> 29 нояб. 2017 г., в 22:50, Shubham Barai <shubhambaraiss@gmail.com>
>> написал(а):
>>
>>  I have fixed formatting style. Please take a look at updated patch.
>>
>>
>> Here's rebased patch. Every issue has been addressed, so I'm marking this
>> patch as ready for committer.
>>
>
> I'm sorry for concentrating on boring things, but formatting of
> predicate-gist.spec still doesn't look good for me.
>
> # To verify serialization failures, queries and permutations are written
>> in such
>> # a way that an index scan(from one transaction) and an index insert(from
>> another
>> # transaction) will try to access the same part(sub-tree) of the index.
>> #
>> # To check reduced false positives, queries and permutations are written
>> in such
>> # a way that an index scan(from one transaction) and an index insert(from
>> another
>> # transaction) will try to access different parts(sub-tree) of the index.
>>
>
> No space before open bracket (I think it should be when there are multiple
> words brackets).
> Also, we're trying to fit our lines to 80 characters (if it's not
> objectively difficult).
> And these are two almost same paragraphs.  I think it should be simplified.
>
> setup
>> {
>>  create table gist_point_tbl(id int4, p point);
>>  create index gist_pointidx on gist_point_tbl using gist(p);
>>  insert into gist_point_tbl (id, p)
>>  select g, point(g*10, g*10) from generate_series(1, 1000) g;
>> }
>> setup
>> {
>>   BEGIN ISOLATION LEVEL SERIALIZABLE;
>>   set enable_seqscan=off;
>>   set enable_bitmapscan=off;
>>   set enable_indexonlyscan=on;
>> }
>> setup {
>>   BEGIN ISOLATION LEVEL SERIALIZABLE;
>>   set enable_seqscan=off;
>>   set enable_bitmapscan=off;
>>   set enable_indexonlyscan=on;
>> }
>
>
> I didn't get idea of using various indentation styles for same purpose.
>
> step "wx3" { insert into gist_point_tbl (id, p)
>>                   select g, point(g*500, g*500) from generate_series(12,
>> 18) g; }
>
>
> Indented using spaces here...
>
>
I have fixed formatting issues. Please have a look at updated patch.


Regards,
Shubham

  22. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Shubham Barai <shubhambaraiss@gmail.com> — 2018-01-08T17:43:36Z

    On 8 January 2018 at 22:44, Shubham Barai <shubhambaraiss@gmail.com> wrote:

>
>
> On 5 January 2018 at 03:18, Alexander Korotkov <a.korotkov@postgrespro.ru>
> wrote:
>
>> On Thu, Jan 4, 2018 at 7:07 PM, Andrey Borodin <x4mmm@yandex-team.ru>
>> wrote:
>>
>>> 29 нояб. 2017 г., в 22:50, Shubham Barai <shubhambaraiss@gmail.com>
>>> написал(а):
>>>
>>>  I have fixed formatting style. Please take a look at updated patch.
>>>
>>>
>>> Here's rebased patch. Every issue has been addressed, so I'm marking
>>> this patch as ready for committer.
>>>
>>
>> I'm sorry for concentrating on boring things, but formatting of
>> predicate-gist.spec still doesn't look good for me.
>>
>> # To verify serialization failures, queries and permutations are written
>>> in such
>>> # a way that an index scan(from one transaction) and an index
>>> insert(from another
>>> # transaction) will try to access the same part(sub-tree) of the index.
>>> #
>>> # To check reduced false positives, queries and permutations are written
>>> in such
>>> # a way that an index scan(from one transaction) and an index
>>> insert(from another
>>> # transaction) will try to access different parts(sub-tree) of the index.
>>>
>>
>> No space before open bracket (I think it should be when there are
>> multiple words brackets).
>> Also, we're trying to fit our lines to 80 characters (if it's not
>> objectively difficult).
>> And these are two almost same paragraphs.  I think it should be
>> simplified.
>>
>> setup
>>> {
>>>  create table gist_point_tbl(id int4, p point);
>>>  create index gist_pointidx on gist_point_tbl using gist(p);
>>>  insert into gist_point_tbl (id, p)
>>>  select g, point(g*10, g*10) from generate_series(1, 1000) g;
>>> }
>>> setup
>>> {
>>>   BEGIN ISOLATION LEVEL SERIALIZABLE;
>>>   set enable_seqscan=off;
>>>   set enable_bitmapscan=off;
>>>   set enable_indexonlyscan=on;
>>> }
>>> setup {
>>>   BEGIN ISOLATION LEVEL SERIALIZABLE;
>>>   set enable_seqscan=off;
>>>   set enable_bitmapscan=off;
>>>   set enable_indexonlyscan=on;
>>> }
>>
>>
>> I didn't get idea of using various indentation styles for same purpose.
>>
>> step "wx3" { insert into gist_point_tbl (id, p)
>>>                   select g, point(g*500, g*500) from generate_series(12,
>>> 18) g; }
>>
>>
>> Indented using spaces here...
>>
>>
>
>
I have fixed formatting issues. Please have a look at updated patch.


Regards,
Shubham

  23. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Shubham Barai <shubhambaraiss@gmail.com> — 2018-01-10T18:55:57Z

    On 8 January 2018 at 23:13, Shubham Barai <shubhambaraiss@gmail.com> wrote:

>
>
> On 8 January 2018 at 22:44, Shubham Barai <shubhambaraiss@gmail.com>
> wrote:
>
>>
>>
>> On 5 January 2018 at 03:18, Alexander Korotkov <a.korotkov@postgrespro.ru
>> > wrote:
>>
>>> On Thu, Jan 4, 2018 at 7:07 PM, Andrey Borodin <x4mmm@yandex-team.ru>
>>> wrote:
>>>
>>>> 29 нояб. 2017 г., в 22:50, Shubham Barai <shubhambaraiss@gmail.com>
>>>> написал(а):
>>>>
>>>>  I have fixed formatting style. Please take a look at updated patch.
>>>>
>>>>
>>>> Here's rebased patch. Every issue has been addressed, so I'm marking
>>>> this patch as ready for committer.
>>>>
>>>
>>> I'm sorry for concentrating on boring things, but formatting of
>>> predicate-gist.spec still doesn't look good for me.
>>>
>>> # To verify serialization failures, queries and permutations are written
>>>> in such
>>>> # a way that an index scan(from one transaction) and an index
>>>> insert(from another
>>>> # transaction) will try to access the same part(sub-tree) of the index.
>>>> #
>>>> # To check reduced false positives, queries and permutations are
>>>> written in such
>>>> # a way that an index scan(from one transaction) and an index
>>>> insert(from another
>>>> # transaction) will try to access different parts(sub-tree) of the
>>>> index.
>>>>
>>>
>>> No space before open bracket (I think it should be when there are
>>> multiple words brackets).
>>> Also, we're trying to fit our lines to 80 characters (if it's not
>>> objectively difficult).
>>> And these are two almost same paragraphs.  I think it should be
>>> simplified.
>>>
>>> setup
>>>> {
>>>>  create table gist_point_tbl(id int4, p point);
>>>>  create index gist_pointidx on gist_point_tbl using gist(p);
>>>>  insert into gist_point_tbl (id, p)
>>>>  select g, point(g*10, g*10) from generate_series(1, 1000) g;
>>>> }
>>>> setup
>>>> {
>>>>   BEGIN ISOLATION LEVEL SERIALIZABLE;
>>>>   set enable_seqscan=off;
>>>>   set enable_bitmapscan=off;
>>>>   set enable_indexonlyscan=on;
>>>> }
>>>> setup {
>>>>   BEGIN ISOLATION LEVEL SERIALIZABLE;
>>>>   set enable_seqscan=off;
>>>>   set enable_bitmapscan=off;
>>>>   set enable_indexonlyscan=on;
>>>> }
>>>
>>>
>>> I didn't get idea of using various indentation styles for same purpose.
>>>
>>> step "wx3" { insert into gist_point_tbl (id, p)
>>>>                   select g, point(g*500, g*500) from
>>>> generate_series(12, 18) g; }
>>>
>>>
>>> Indented using spaces here...
>>>
>>>
>>
>>
> I have fixed formatting issues. Please have a look at updated patch.
>
>
  The previous patch couldn't be applied cleanly because there were some
 modifications to isolation_schedule. I have updated the patch now.


Regards,
Shubham

  24. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2018-01-25T13:10:26Z

    On Wed, Jan 10, 2018 at 9:55 PM, Shubham Barai <shubhambaraiss@gmail.com>
wrote:

>  The previous patch couldn't be applied cleanly because there were some
>  modifications to isolation_schedule. I have updated the patch now.
>

In the attached patch indentation is still looking strange.
I've contacted Shubham using Telegram, and we realized that
it's because he used tab width 8 in his editor.
Shumham seems to have updated version of this patch, but didn't
post it yet.  Thus, I'm marking this "Waiting on author" until
the updated patch is posted.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  25. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Shubham Barai <shubhambaraiss@gmail.com> — 2018-01-25T14:13:35Z

    On 25 January 2018 at 18:40, Alexander Korotkov <a.korotkov@postgrespro.ru>
wrote:

> On Wed, Jan 10, 2018 at 9:55 PM, Shubham Barai <shubhambaraiss@gmail.com>
> wrote:
>
>>  The previous patch couldn't be applied cleanly because there were some
>>  modifications to isolation_schedule. I have updated the patch now.
>>
>
> In the attached patch indentation is still looking strange.
> I've contacted Shubham using Telegram, and we realized that
> it's because he used tab width 8 in his editor.
> Shumham seems to have updated version of this patch, but didn't
> post it yet.  Thus, I'm marking this "Waiting on author" until
> the updated patch is posted.
>
>
>
I have fixed formatting issues. Please take a look at updated patch.

Regards,
Shubham

  26. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2018-01-25T14:32:45Z

    On Thu, Jan 25, 2018 at 5:13 PM, Shubham Barai <shubhambaraiss@gmail.com>
wrote:

>
>
> On 25 January 2018 at 18:40, Alexander Korotkov <a.korotkov@postgrespro.ru
> > wrote:
>
>> On Wed, Jan 10, 2018 at 9:55 PM, Shubham Barai <shubhambaraiss@gmail.com>
>> wrote:
>>
>>>  The previous patch couldn't be applied cleanly because there were some
>>>  modifications to isolation_schedule. I have updated the patch now.
>>>
>>
>> In the attached patch indentation is still looking strange.
>> I've contacted Shubham using Telegram, and we realized that
>> it's because he used tab width 8 in his editor.
>> Shumham seems to have updated version of this patch, but didn't
>> post it yet.  Thus, I'm marking this "Waiting on author" until
>> the updated patch is posted.
>>
>>
>>
> I have fixed formatting issues. Please take a look at updated patch.
>

Now it looks good for me.  I'm marking it "Ready for committer".

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  27. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Teodor Sigaev <teodor@sigaev.ru> — 2018-03-27T07:53:12Z

    Hi!

> Now it looks good for me.  I'm marking it "Ready for committer".

I have a question: why do not CheckForSerializableConflictIn() move  into 
begining of gistplacetopage()? Seems, it is the single function which actually 
changes page and all predicate locking stuff will be placed in single function...

See attached version of patch.
-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

  28. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    x4mmm@yandex-team.ru — 2018-03-27T08:16:19Z

    Hi1

> 27 марта 2018 г., в 12:53, Teodor Sigaev <teodor@sigaev.ru> написал(а):
> 
> I have a question: why do not CheckForSerializableConflictIn() move  into begining of gistplacetopage()? Seems, it is the single function which actually changes page and all predicate locking stuff will be placed in single function...

gistplacetopage() is called from
1. Buffered build - probably harmless
2. Finish split - i'm not sure about this. It seems to me that it is necessary... then your version is correct.

Best regards, Andrey Borodin.

  29. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2018-03-27T08:45:32Z

    Hi!

On Tue, Mar 27, 2018 at 11:16 AM, Andrey Borodin <x4mmm@yandex-team.ru>
wrote:

> > 27 марта 2018 г., в 12:53, Teodor Sigaev <teodor@sigaev.ru> написал(а):
> >
> > I have a question: why do not CheckForSerializableConflictIn() move
> into begining of gistplacetopage()? Seems, it is the single function which
> actually changes page and all predicate locking stuff will be placed in
> single function...
>
> gistplacetopage() is called from
> 1. Buffered build - probably harmless
>

Yes, harmless, but useless.


> 2. Finish split - i'm not sure about this. It seems to me that it is
> necessary... then your version is correct.
>

Yes, it's necessary, because GiST scan can end up on non-leaf page.  So,
scan and modify of same non-leaf page should conflict.

Checking for serializable conflicts from buffering build seems useless
overhead.  gistplacetopage()
is called from only two places: gistinserttuples()
and gistbufferinginserttuples().  In order to evade
useless overhead for buffering build, I've moved
CheckForSerializableConflictIn() into gistinserttuples().

Also, I find that we call PredicateLockPageSplit() for every page produced
by split including
original.  That also seems to cause extra overhead.  This is why I've moved
PredicateLockPageSplit() into loop where we do assign new buffers.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  30. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    x4mmm@yandex-team.ru — 2018-03-27T09:07:55Z

    
> 27 марта 2018 г., в 13:45, Alexander Korotkov <a.korotkov@postgrespro.ru> написал(а):
> 
> On Tue, Mar 27, 2018 at 11:16 AM, Andrey Borodin <x4mmm@yandex-team.ru <mailto:x4mmm@yandex-team.ru>> wrote:
> > 27 марта 2018 г., в 12:53, Teodor Sigaev <teodor@sigaev.ru <mailto:teodor@sigaev.ru>> написал(а):
> >
> > I have a question: why do not CheckForSerializableConflictIn() move  into begining of gistplacetopage()? Seems, it is the single function which actually changes page and all predicate locking stuff will be placed in single function...
> 
> gistplacetopage() is called from
> 1. Buffered build - probably harmless
> 
> Yes, harmless, but useless.
>  
> 2. Finish split - i'm not sure about this. It seems to me that it is necessary... then your version is correct.
> 
> Yes, it's necessary, because GiST scan can end up on non-leaf page.  So, scan and modify of same non-leaf page should conflict.
> 
> Checking for serializable conflicts from buffering build seems useless overhead.  gistplacetopage()
> is called from only two places: gistinserttuples() and gistbufferinginserttuples().  In order to evade
> useless overhead for buffering build, I've moved CheckForSerializableConflictIn() into gistinserttuples().
+1
Also, both gistdoinsert() and gistplacetopage() are mind blowing in complexity. gistinserttuples() looks like a cosy place if we need to add some more.

Best regards, Andrey Borodin.

  31. Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

    Teodor Sigaev <teodor@sigaev.ru> — 2018-03-27T12:44:48Z

    Thanks to everyone, pushed

Andrey Borodin wrote:
> 
> 
>> 27 марта 2018 г., в 13:45, Alexander Korotkov <a.korotkov@postgrespro.ru 
>> <mailto:a.korotkov@postgrespro.ru>> написал(а):
>>
>> On Tue, Mar 27, 2018 at 11:16 AM, Andrey Borodin <x4mmm@yandex-team.ru 
>> <mailto:x4mmm@yandex-team.ru>> wrote:
>>
>>     > 27 марта 2018 г., в 12:53, Teodor Sigaev <teodor@sigaev.ru
>>     <mailto:teodor@sigaev.ru>> написал(а):
>>     >
>>     > I have a question: why do not CheckForSerializableConflictIn() move  into begining of gistplacetopage()? Seems, it is the single
>>     function which actually changes page and all predicate locking stuff will
>>     be placed in single function...
>>
>>     gistplacetopage() is called from
>>     1. Buffered build - probably harmless
>>
>>
>> Yes, harmless, but useless.
>>
>>     2. Finish split - i'm not sure about this. It seems to me that it is
>>     necessary... then your version is correct.
>>
>>
>> Yes, it's necessary, because GiST scan can end up on non-leaf page.  So, scan 
>> and modify of same non-leaf page should conflict.
>>
>> Checking for serializable conflicts from buffering build seems useless 
>> overhead.  gistplacetopage()
>> is called from only two places: gistinserttuples() 
>> and gistbufferinginserttuples().  In order to evade
>> useless overhead for buffering build, I've moved 
>> CheckForSerializableConflictIn() into gistinserttuples().
> +1
> Also, both gistdoinsert() and gistplacetopage() are mind blowing in complexity. 
> gistinserttuples() looks like a cosy place if we need to add some more.
> 
> Best regards, Andrey Borodin.

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/