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

[Teodor Sigaev]

Thanks to everyone, pushed

Andrey Borodin wrote:



27 марта 2018 г., в 13:45, Alexander Korotkov > написал(а):


On Tue, Mar 27, 2018 at 11:16 AM, Andrey Borodin > wrote:


> 27 марта 2018 г., в 12:53, Teodor Sigaev > написал(а):
>
> 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: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

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

[Andrey Borodin]

> 27 марта 2018 г., в 13:45, Alexander Korotkov  
> написал(а):
> 
> On Tue, Mar 27, 2018 at 11:16 AM, Andrey Borodin  > wrote:
> > 27 марта 2018 г., в 12:53, Teodor Sigaev  > > написал(а):
> >
> > 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.

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

[Alexander Korotkov]

Hi!

On Tue, Mar 27, 2018 at 11:16 AM, Andrey Borodin 
wrote:

> > 27 марта 2018 г., в 12:53, Teodor Sigaev  написал(а):
> >
> > 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


Predicate-Locking-in-gist-index_v11.patch
Description: Binary data

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

[Andrey Borodin]

Hi1

> 27 марта 2018 г., в 12:53, Teodor Sigaev  написал(а):
> 
> 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.

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

[Teodor Sigaev]

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: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 51c32e4afe..5ee1e5e4e1 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -18,6 +18,8 @@
 #include "access/gistscan.h"
 #include "catalog/pg_collation.h"
 #include "miscadmin.h"
+#include "storage/lmgr.h"
+#include "storage/predicate.h"
 #include "nodes/execnodes.h"
 #include "utils/builtins.h"
 #include "utils/index_selfuncs.h"
@@ -70,7 +72,7 @@ gisthandler(PG_FUNCTION_ARGS)
 	amroutine->amsearchnulls = true;
 	amroutine->amstorage = true;
 	amroutine->amclusterable = true;
-	amroutine->ampredlocks = false;
+	amroutine->ampredlocks = true;
 	amroutine->amcanparallel = false;
 	amroutine->amkeytype = InvalidOid;
 
@@ -224,6 +226,12 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 	int			i;
 	bool		is_split;
 
+	/*
+	 * Check for any rw conflicts (in serialisation isolation level)
+	 * just before we intend to modify the page
+	 */
+	CheckForSerializableConflictIn(rel, NULL, buffer);
+
 	/*
 	 * Refuse to modify a page that's incompletely split. This should not
 	 * happen because we finish any incomplete splits while we walk down the
@@ -446,6 +454,11 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 			GistPageSetNSN(ptr->page, oldnsn);
 		}
 
+		for (ptr = dist; ptr; ptr = ptr->next)
+			PredicateLockPageSplit(rel,
+		BufferGetBlockNumber(buffer),
+		BufferGetBlockNumber(ptr->buffer));
+
 		/*
 		 * gistXLogSplit() needs to WAL log a lot of pages, prepare WAL
 		 * insertion for that. NB: The number of pages and data segments
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index b30b931c3b..c4e8a3b913 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -18,6 +18,8 @@
 #include "access/relscan.h"
 #include "catalog/pg_type.h"
 #include "miscadmin.h"
+#include "storage/lmgr.h"
+#include "storage/predicate.h"
 #include "pgstat.h"
 #include "lib/pairingheap.h"
 #include "utils/builtins.h"
@@ -336,6 +338,7 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem, double *myDistances,
 
 	buffer = ReadBuffer(scan->indexRelation, pageItem->blkno);
 	LockBuffer(buffer, GIST_SHARE);
+	PredicateLockPage(r, BufferGetBlockNumber(buffer), scan->xs_snapshot);
 	gistcheckpage(scan->indexRelation, buffer);
 	page = BufferGetPage(buffer);
 	TestForOldSnapshot(scan->xs_snapshot, r, page);
diff --git a/src/backend/storage/lmgr/README-SSI b/src/backend/storage/lmgr/README-SSI
index a9dc01f237..e221241f96 100644
--- a/src/backend/storage/lmgr/README-SSI
+++ b/src/backend/storage/lmgr/README-SSI
@@ -374,10 +374,11 @@ however, a search discovers that no root page has yet been created, a
 predicate lock on the index relation is required.
 
 * GiST searches can determine that there are no matches at any
-level of the index, so there must be a predicate lock at each index
+level of the index, so we acquire predicate lock at each index
 level during a GiST search. An index insert at the leaf level can
 then be trusted to ripple up to all levels and locations where
-conflicting predicate locks may exist.
+conflicting predicate locks may exist. In case there is a page split,
+we need to copy predicate lock from an original page to all new pages.
 
 * The effects of page splits, overflows, consolidations, and
 removals must be carefully reviewed to ensure that predicate locks
diff --git a/src/test/isolation/expected/predicate-gist.out b/src/test/isolation/expected/predicate-gist.out
new file mode 100644
index 00..77a27958af
--- /dev/null
+++ b/src/test/isolation/expected/predicate-gist.out
@@ -0,0 +1,659 @@
+Parsed test spec with 2 sessions
+
+starting permutation: rxy1 wx1 c1 rxy2 wy2 c2
+step rxy1: select sum(p[0]) from gist_point_tbl where p << point(2500, 2500);
+sum
+
+311250 
+step wx1: insert into gist_point_tbl (id, p)
+			  select g, point(g*500, g*500) from generate_series(15, 20) g;
+step c1: commit;
+step rxy2: select sum(p[0]) from gist_point_tbl where p >> point(7500,7500);
+sum
+
+2233750
+step wy2: insert into gist_point_tbl (id, p)
+			  select g, point(g*500, g*500) from generate_series(1, 5) g;
+step c2: commit;
+
+starting permutation: rxy2 wy2 c2 rxy1 wx1 c1
+step rxy2: select sum(p[0]) from gist_point_tbl where p >> point(7500,7500);
+sum
+
+2188750
+step wy2: insert into gist_point_tbl (id, p)
+			  select 

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

[Alexander Korotkov]

On Thu, Jan 25, 2018 at 5:13 PM, Shubham Barai 
wrote:

>
>
> On 25 January 2018 at 18:40, Alexander Korotkov  > wrote:
>
>> On Wed, Jan 10, 2018 at 9:55 PM, Shubham Barai 
>> 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

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

[Shubham Barai]

On 25 January 2018 at 18:40, Alexander Korotkov 
wrote:

> On Wed, Jan 10, 2018 at 9:55 PM, Shubham Barai 
> 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


Predicate-Locking-in-gist-index_v9.patch
Description: Binary data

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

[Alexander Korotkov]

On Wed, Jan 10, 2018 at 9:55 PM, Shubham Barai 
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

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

[Shubham Barai]

On 8 January 2018 at 22:44, Shubham Barai  wrote:

>
>
> On 5 January 2018 at 03:18, Alexander Korotkov 
> wrote:
>
>> On Thu, Jan 4, 2018 at 7:07 PM, Andrey Borodin 
>> wrote:
>>
>>> 29 нояб. 2017 г., в 22:50, Shubham Barai 
>>> написал(а):
>>>
>>>  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


Predicate-locking-in-gist_index_v7.patch
Description: Binary data

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

[Shubham Barai]

On 5 January 2018 at 03:18, Alexander Korotkov 
wrote:

> On Thu, Jan 4, 2018 at 7:07 PM, Andrey Borodin 
> wrote:
>
>> 29 нояб. 2017 г., в 22:50, Shubham Barai 
>> написал(а):
>>
>>  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


Predicate-locking-in-gist-index_7.patch
Description: Binary data

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

[Andrey Borodin]

Hello everyone!29 нояб. 2017 г., в 22:50, Shubham Barai  написал(а): 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.

0001-Predicate-locking-in-GiST-index-v6.patch
Description: Binary data

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

[Michael Paquier]

On Mon, Nov 27, 2017 at 4:47 PM, Alexander Korotkov
 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

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

[Alexander Korotkov]

Hi, Shubham!

On Wed, Nov 1, 2017 at 12:10 AM, Shubham Barai 
wrote:

> On 9 October 2017 at 18:57, Alexander Korotkov 
> 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