Thread: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

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

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.

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

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.

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

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;

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.

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.

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

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.

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.

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.

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

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

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.

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.

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.

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