Обсуждение: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index
![](https://mailtrack.io/trace/mail/e27a432cb776f8e0c24c9d29b372aadfe2ea6611.png?u=1817942)
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
Вложения
Hi,
Please find the updated patch here.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
Вложения
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
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
![](https://mailtrack.io/trace/mail/88d9b7025db7e1455920d12bcd8b968ebb59db0f.png?u=1817942)
On 21 June 2017 at 13:11, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I know that. This is the old version of the patch. I had sent updated patch later. Please have a look at updated patch.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
Regards,
Shubham
Вложения
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.
Hi!
On Wed, Jun 21, 2017 at 10:52 AM, Shubham Barai <shubhambaraiss@gmail.com> wrote:
I took a look on this patch.
Hi,
On 21 June 2017 at 13:11, Heikki Linnakangas <hlinnaka@iki.fi> wrote:I know that. This is the old version of the patch. I had sent updated patch later. Please have a look at updated patch.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
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;
The Russian Postgres Company
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). However, page might be exclusively locked before. And in this case CheckForSerializableConflictInif (!xlocked)
{
LockBuffer(stack->buffer, GIST_UNLOCK);
LockBuffer(stack->buffer, GIST_EXCLUSIVE);
CheckForSerializableConflictIn(r, NULL, stack->buffer);
xlocked = true;() 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.
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:I took a look on this patch.Hi,
On 21 June 2017 at 13:11, Heikki Linnakangas <hlinnaka@iki.fi> wrote:I know that. This is the old version of the patch. I had sent updated patch later. Please have a look at updated patch.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.
- HeikkiIn gistdoinsert() you do CheckForSerializableConflictIn() only if page wasn't exclusively locked before (xlocked is false). However, page might be exclusively locked before. And in this case CheckForSerializableConflictInif (!xlocked)
{
LockBuffer(stack->buffer, GIST_UNLOCK);
LockBuffer(stack->buffer, GIST_EXCLUSIVE);
CheckForSerializableConflictIn(r, NULL, stack->buffer);
xlocked = true;() 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
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). However, page might be exclusively locked before. And in this case CheckForSerializableConflictInif (!xlocked)
{
LockBuffer(stack->buffer, GIST_UNLOCK);
LockBuffer(stack->buffer, GIST_EXCLUSIVE);
CheckForSerializableConflictIn(r, NULL, stack->buffer);
xlocked = true;() 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
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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
![](https://mailtrack.io/trace/mail/1a91701b3411ce6ff12fc7515de0bbfe9a326256.png?u=1817942)
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
Вложения
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.
The Russian Postgres Company
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
Вложения
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
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
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
Вложения
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.
Вложения
On Thu, Jan 4, 2018 at 7:07 PM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
I didn't get idea of using various indentation styles for same purpose.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
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
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
Вложения
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 Вложения
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
Вложения
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 somemodifications 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.
The Russian Postgres Company
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 somemodifications 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 thatit's because he used tab width 8 in his editor.Shumham seems to have updated version of this patch, but didn'tpost it yet. Thus, I'm marking this "Waiting on author" untilthe updated patch is posted.
I have fixed formatting issues. Please take a look at updated patch.
Regards,
Shubham
Вложения
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 somemodifications 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 thatit's because he used tab width 8 in his editor.Shumham seems to have updated version of this patch, but didn'tpost it yet. Thus, I'm marking this "Waiting on author" untilthe 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".
The Russian Postgres Company
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/
Вложения
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 thesingle 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.
> 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
Alexander Korotkov
Postgres Professional: http://www.
The Russian Postgres Company
Вложения
+127 марта 2018 г., в 13:45, Alexander Korotkov <a.korotkov@postgrespro.ru> написал(а):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 harmlessYes, 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 movedCheckForSerializableConflictIn () into gistinserttuples().
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.
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/