Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

Kevin Grittner wrote:

Simon Riggs wrote:

On 6 October 2012 00:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:

 

2. DROP INDEX CONCURRENTLY doesn't bother to do
TransferPredicateLocksToHeapRelation until long after it's
invalidated the index. Surely that's no good? Is it even possible
to do that correctly, when we don't have a lock that will prevent
new predicate locks from being taken out meanwhile?

No idea there. Input appreciated.

If the creation of a new tuple by insert or update would not
perform the related index tuple insertion, and the lock has not yet
been transfered to the heap relation, yes we have a problem.  Will
take a look at the code.

Creation of new predicate locks while in this state has no bearing
on the issue as long as locks are transferred to the heap relation
after the last scan using the index has completed.

To put that another way, it should be done at a time when it is sure
that no query sees indisvalid = true and no query has yet seen
indisready = false.  Patch attached.  Will apply if nobody sees a
problem with it.

-Kevin

"Kevin Grittner" <kgrittn@mail.com> writes:

To put that another way, it should be done at a time when it is sure
that no query sees indisvalid = true and no query has yet seen
indisready = false.  Patch attached.  Will apply if nobody sees a
problem with it.

The above statement of the requirement doesn't seem to match what you
put in the comment:

+ 	 * All predicate locks on the index are about to be made invalid. Promote
+ 	 * them to relation locks on the heap. For correctness this must be done
+ 	 * after the index was last seen with indisready = true and before it is
+ 	 * seen with indisvalid = false.

and the comment is rather vaguely worded too (last seen by what?).
Please wordsmith that a bit more.

regards, tom lane

Tom Lane wrote:

"Kevin Grittner" <kgrittn@mail.com> writes:

To put that another way, it should be done at a time when it is
sure that no query sees indisvalid = true and no query has yet
seen indisready = false. Patch attached. Will apply if nobody sees
a problem with it.

The above statement of the requirement doesn't seem to match what
you put in the comment:

+ * All predicate locks on the index are about to be made invalid. Promote
+ * them to relation locks on the heap. For correctness this must be done
+ * after the index was last seen with indisready = true and before it is
+ * seen with indisvalid = false.

It took me a while to spot it, but yeah -- I reversed the field names
in the comment. :-( The email was right; I fixed the comment.

and the comment is rather vaguely worded too (last seen by what?).
Please wordsmith that a bit more.

I tried to leave nothing to the imagination this time.

I think the README or function comments on the predicate locking
should include more about this sort of thing. In retrospect, the
documentation reads more like a maintainer's guide for SSI, and the
user's guide is lacking. Something like that would make it easier for
people working on other features (like DROP INDEX CONCURRENTLY) to
know where to put which calls. That's more than I can do right at the
moment, but I'll make a note of it. I suspect that if enough of this
is in the comment above the function definition, less of it needs to
be near each call site.

-Kevin

Kevin Grittner wrote:

It took me a while to spot it, but yeah -- I reversed the field
names in the comment. :-( The email was right; I fixed the comment.

Hmm. The comment is probably better now, but I've been re-checking
the code, and I think my actual code change is completely wrong. Give
me a bit to sort this out.

-Kevin

Kevin Grittner wrote:

Hmm. The comment is probably better now, but I've been re-checking
the code, and I think my actual code change is completely wrong.
Give me a bit to sort this out.

I'm having trouble seeing a way to make this work without rearranging
the code for concurrent drop to get to a state where it has set
indisvalid = false, made that visible to all processes, and ensured
that all scans of the index are complete -- while indisready is still
true. That is the point where TransferPredicateLocksToHeapRelation()
could be safely called. Then we would need to set indisready = false,
make that visible to all processes, and ensure that all access to the
index is complete. I can't see where it works to set both flags at
the same time. I want to sleep on it to see if I can come up with any
other way, but right now that's the only way I'm seeing to make DROP
INDEX CONCURRENTLY compatible with SERIALIZABLE transactions. :-(

-Kevin

On Thursday, October 18, 2012 06:12:02 AM Kevin Grittner wrote:

Kevin Grittner wrote:

Hmm. The comment is probably better now, but I've been re-checking
the code, and I think my actual code change is completely wrong.
Give me a bit to sort this out.

I'm having trouble seeing a way to make this work without rearranging
the code for concurrent drop to get to a state where it has set
indisvalid = false, made that visible to all processes, and ensured
that all scans of the index are complete -- while indisready is still
true. That is the point where TransferPredicateLocksToHeapRelation()
could be safely called. Then we would need to set indisready = false,
make that visible to all processes, and ensure that all access to the
index is complete. I can't see where it works to set both flags at
the same time. I want to sleep on it to see if I can come up with any
other way, but right now that's the only way I'm seeing to make DROP
INDEX CONCURRENTLY compatible with SERIALIZABLE transactions. :-(

In a nearby bug I had to restructure the code that in a way thats similar to
this anyway, so that seems fine. Maybe you can fix the bug ontop of the two
attached patches?

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund wrote:

On Thursday, October 18, 2012 06:12:02 AM Kevin Grittner wrote:

I'm having trouble seeing a way to make this work without
rearranging the code for concurrent drop to get to a state where
it has set indisvalid = false, made that visible to all processes,
and ensured that all scans of the index are complete -- while
indisready is still true. That is the point where
TransferPredicateLocksToHeapRelation() could be safely called.
Then we would need to set indisready = false, make that visible to
all processes, and ensure that all access to the index is
complete. I can't see where it works to set both flags at the same
time.

In a nearby bug I had to restructure the code that in a way thats
similar to this anyway, so that seems fine. Maybe you can fix the
bug ontop of the two attached patches?

Perfect; these two patches provide a spot in the code which is
exactly right for handling the predicate lock adjustments. Attached
is a patch which applies on top of the two you sent.

Thanks!

-Kevin

On 18 October 2012 10:20, Andres Freund <andres@2ndquadrant.com> wrote:

On Thursday, October 18, 2012 06:12:02 AM Kevin Grittner wrote:

Kevin Grittner wrote:

Hmm. The comment is probably better now, but I've been re-checking
the code, and I think my actual code change is completely wrong.
Give me a bit to sort this out.

I'm having trouble seeing a way to make this work without rearranging
the code for concurrent drop to get to a state where it has set
indisvalid = false, made that visible to all processes, and ensured
that all scans of the index are complete -- while indisready is still
true. That is the point where TransferPredicateLocksToHeapRelation()
could be safely called. Then we would need to set indisready = false,
make that visible to all processes, and ensure that all access to the
index is complete. I can't see where it works to set both flags at
the same time. I want to sleep on it to see if I can come up with any
other way, but right now that's the only way I'm seeing to make DROP
INDEX CONCURRENTLY compatible with SERIALIZABLE transactions. :-(

In a nearby bug I had to restructure the code that in a way thats similar to
this anyway, so that seems fine. Maybe you can fix the bug ontop of the two
attached patches?

First patch and first test committed.

Working on second patch/test.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 18 October 2012 19:48, Simon Riggs <simon@2ndquadrant.com> wrote:

On 18 October 2012 10:20, Andres Freund <andres@2ndquadrant.com> wrote:

On Thursday, October 18, 2012 06:12:02 AM Kevin Grittner wrote:

Kevin Grittner wrote:

Hmm. The comment is probably better now, but I've been re-checking
the code, and I think my actual code change is completely wrong.
Give me a bit to sort this out.

I'm having trouble seeing a way to make this work without rearranging
the code for concurrent drop to get to a state where it has set
indisvalid = false, made that visible to all processes, and ensured
that all scans of the index are complete -- while indisready is still
true. That is the point where TransferPredicateLocksToHeapRelation()
could be safely called. Then we would need to set indisready = false,
make that visible to all processes, and ensure that all access to the
index is complete. I can't see where it works to set both flags at
the same time. I want to sleep on it to see if I can come up with any
other way, but right now that's the only way I'm seeing to make DROP
INDEX CONCURRENTLY compatible with SERIALIZABLE transactions. :-(

In a nearby bug I had to restructure the code that in a way thats similar to
this anyway, so that seems fine. Maybe you can fix the bug ontop of the two
attached patches?

First patch and first test committed.

Working on second patch/test.

I've applied the second patch as-is.

The second test shows it passes, but the nature of the bug is fairly
obscure, so having a specific test for dropping an already dropped
object is a little strange and so I've not applied that.

Thanks for fixes and tests.

Kevin, you're good to go on the SSI patch, or I'll apply next week if
you don't. Thanks for that.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs wrote:

Kevin, you're good to go on the SSI patch, or I'll apply next week
if you don't. Thanks for that.

There were some hunks failing because of minor improvements to the
comments you applied, so attached is a version with trivial
adjustments for that.  Will apply tomorrow if there are no further
objections.

Nice feature, BTW!

-Kevin

Kevin Grittner wrote:

Will apply tomorrow if there are no further objections.

Done.

-Kevin

"Kevin Grittner" <kgrittn@mail.com> writes:

Kevin Grittner wrote:

Will apply tomorrow if there are no further objections.

Done.

This needs to be back-patched, no?

regards, tom lane

Tom Lane wrote:

This needs to be back-patched, no?

Looking at that now.

-Kevin

Kevin Grittner wrote:

Tom Lane wrote:

This needs to be back-patched, no?

Looking at that now.

Back-patched to 9.2.  I don't know how I got it in my head that this
was a pending 9.3 feature.  I'll check next time, even if I think I
know.

Thanks to both Andres and Tom for pointing that out.

-Kevin