GSoC 2017 : Patch for predicate locking in Gist index

Hi,

Please find the updated patch here.

Regards,
Shubham

On 16 June 2017 at 15:54, Shubham Barai <shubhambaraiss@gmail.com> 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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/&gt; Sent with Mailtrack
<https://mailtrack.io/install?source=signature&amp;lang=en&amp;referral=shubhambaraiss@gmail.com&amp;idSignature=22&gt;

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:

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

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.

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

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

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/&gt; Sent with Mailtrack
<https://chrome.google.com/webstore/detail/mailtrack-for-gmail-inbox/ndnaehgpjlnokgebbaldlmgkapkpjkkb?utm_source=gmail&amp;utm_medium=signature&amp;utm_campaign=signaturevirality&gt;
<#>

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.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
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 <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

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:

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