Re: SIREAD lock versus ACCESS EXCLUSIVE lock

Composing my rather long-winded response to Simon got me thinking --
which just led me to realize there is probably a need to fix another
thing related to SSI.

For correct serializable behavior in the face of concurrent DDL
execution, I think that a request for a heavyweight ACCESS EXCLUSIVE
lock might need to block until all SIREAD locks on the relation have
been released. Picture, for example, what might happen if one
transaction acquires some predicate locks, then commits (releasing
its heavyweight lock on the table), and before concurrent READ WRITE
transactions complete there is a CLUSTER on the table. Or a DROP
INDEX. :-(

Both require an ACCESS EXCLUSIVE lock. Since an active transaction
would already have an ACCESS SHARE lock when acquiring the SIREAD
locks, this couldn't block in the other direction or with an active
transaction. That means that it couldn't cause any deadlocks if we
added blocking to the acquisition of an ACCESS EXCLUSIVE based on
this.

If we don't do this I don't think that there is a more serious
impact than inaccurate conflict detection for serializable
transactions which are active when these operations are performed.
Well, that and the possibility of seeing SIRead locks in the
pg_locks view for indexes or tables which no longer exist. So far I
don't see any crash modes or effects on non-serializable
transactions. If this change is too destabilizing for this point in
the release we could document it as a limitation and fix it in 9.2.

We're pretty aggressive about cleaning up SIREAD locks as soon as
allowable after transaction completion, so this probably wouldn't
happen very often.

-Kevin

On 27.04.2011 22:59, Kevin Grittner wrote:
> For correct serializable behavior in the face of concurrent DDL
> execution, I think that a request for a heavyweight ACCESS EXCLUSIVE
> lock might need to block until all SIREAD locks on the relation have
> been released. Picture, for example, what might happen if one
> transaction acquires some predicate locks, then commits (releasing
> its heavyweight lock on the table), and before concurrent READ WRITE
> transactions complete there is a CLUSTER on the table. Or a DROP
> INDEX. :-(

Hmm, could we upgrade all predicate locks to relation-level predicate
locks instead?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> On 27.04.2011 22:59, Kevin Grittner wrote:
>> For correct serializable behavior in the face of concurrent DDL
>> execution, I think that a request for a heavyweight ACCESS
>> EXCLUSIVE lock might need to block until all SIREAD locks on the
>> relation have been released. Picture, for example, what might
>> happen if one transaction acquires some predicate locks, then
>> commits (releasing its heavyweight lock on the table), and before
>> concurrent READ WRITE transactions complete there is a CLUSTER on
>> the table. Or a DROP INDEX. :-(
>
> Hmm, could we upgrade all predicate locks to relation-level
> predicate locks instead?

Tied to what backend? This only comes into play with transactions
which have committed and are not yet able to release their predicate
locks because of overlapping READ WRITE serializable transactions
which are still active. Until that point the ACCESS SHARE lock (or
stronger) is still there protecting against this.

One way we could push this entirely into the heavyweight locking
system would be for a committing transaction with predicate locks to
somehow cause all overlapping read write serializable transactions
which are still active to acquire an ACCESS SHARE lock on each
relation for which the committing transaction has any predicate
lock(s). (Unless of course they already hold some lock on a
relevant relation.) This would need to be done before the
committing transaction released its lock on the relation (which it
must have acquired to read something which would cause it to have a
predicate lock). Is that feasible? It seems like a lot of work,
and I'm not sure how one transaction would convince another
process's transaction to acquire the locks.

As an alternative, perhaps we could find a way to leave the relation
locks for a serializable transaction until it's safe to clean up the
predicate locks? They could be downgraded to ACCESS SHARE if they
are stronger. They would need to survive beyond not only the commit
of the transaction, but also the termination of the connection.
They would need to be immune to being chosen as deadlock victims.

Any other ideas?

-Kevin

On Wed, Apr 27, 2011 at 02:59:19PM -0500, Kevin Grittner wrote:
> For correct serializable behavior in the face of concurrent DDL
> execution, I think that a request for a heavyweight ACCESS EXCLUSIVE
> lock might need to block until all SIREAD locks on the relation have
> been released. Picture, for example, what might happen if one

I think you're correct about this, but also agree that it would be
reasonable to document the limitation for now and punt on a fix until
9.2.

On Wed, Apr 27, 2011 at 04:09:38PM -0500, Kevin Grittner wrote:
> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> > Hmm, could we upgrade all predicate locks to relation-level
> > predicate locks instead?
>
> Tied to what backend? This only comes into play with transactions
> which have committed and are not yet able to release their predicate
> locks because of overlapping READ WRITE serializable transactions
> which are still active. Until that point the ACCESS SHARE lock (or
> stronger) is still there protecting against this.

I think Heikki was suggesting to upgrade to relation-level SIREAD
locks. This seems like it would work, but might cause a lot of aborts
immediately afterwards. I do wonder if it might be necessary to upgrade
index locks to relation-level locks on the heap relation, in cases of
dropping the index.

> One way we could push this entirely into the heavyweight locking
> system would be for a committing transaction with predicate locks to
> somehow cause all overlapping read write serializable transactions
> which are still active to acquire an ACCESS SHARE lock on each
> relation for which the committing transaction has any predicate
> lock(s).

I'm not entirely clear on how this would work, but it sounds like it
could also have a significant performance cost.

> As an alternative, perhaps we could find a way to leave the relation
> locks for a serializable transaction until it's safe to clean up the
> predicate locks? They could be downgraded to ACCESS SHARE if they
> are stronger. They would need to survive beyond not only the commit
> of the transaction, but also the termination of the connection.
> They would need to be immune to being chosen as deadlock victims.

This sounds like it would require major changes to the heavyweight lock
manager. There's already a notion of keeping locks past backend
termination for 2PC prepared transactions, but it would be hard to
apply here.

The most practical solutions I see here are to, when acquiring an
AccessExclusive lock, either wait for any associated SIREAD locks to go
away, or to promote them to relation level.

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

Dan Ports <drkp(at)csail(dot)mit(dot)edu> wrote:
> On Wed, Apr 27, 2011 at 04:09:38PM -0500, Kevin Grittner wrote:
>> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>> Hmm, could we upgrade all predicate locks to relation-level
>>> predicate locks instead?
>>
>> Tied to what backend?

> I think Heikki was suggesting to upgrade to relation-level SIREAD
> locks.

Oh, yeah. That *is* what he said, isn't it. That's a simpler
solution which works just fine. Please forget my over-excited
flights of fancy on the topic -- I was so focused on what I thought
was the solution I misread Heikki's much better idea as some
variation on my theme. :-(

-Kevin

On Wed, Apr 27, 2011 at 8:59 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:

> For correct serializable behavior in the face of concurrent DDL
> execution, I think that a request for a heavyweight ACCESS EXCLUSIVE
> lock might need to block until all SIREAD locks on the relation have
> been released.  Picture, for example, what might happen if one
> transaction acquires some predicate locks, then commits (releasing
> its heavyweight lock on the table), and before concurrent READ WRITE
> transactions complete there is a CLUSTER on the table. Or a DROP
> INDEX.  :-(

Sorry, I can't picture it. What will happen?

> Both require an ACCESS EXCLUSIVE lock.  Since an active transaction
> would already have an ACCESS SHARE lock when acquiring the SIREAD
> locks, this couldn't block in the other direction or with an active
> transaction.  That means that it couldn't cause any deadlocks if we
> added blocking to the acquisition of an ACCESS EXCLUSIVE based on
> this.
>
> If we don't do this I don't think that there is a more serious
> impact than inaccurate conflict detection for serializable
> transactions which are active when these operations are performed.
> Well, that and the possibility of seeing SIRead locks in the
> pg_locks view for indexes or tables which no longer exist.  So far I
> don't see any crash modes or effects on non-serializable
> transactions.  If this change is too destabilizing for this point in
> the release we could document it as a limitation and fix it in 9.2.

I don't think this should wait for 9.2

It either works, or it doesn't. Putting caveats in there will just
detract from people's belief in it.

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

Simon Riggs <simon(at)2ndQuadrant(dot)com> wrote:
> On Wed, Apr 27, 2011 at 8:59 PM, Kevin Grittner
> <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
>
>> For correct serializable behavior in the face of concurrent DDL
>> execution, I think that a request for a heavyweight ACCESS
>> EXCLUSIVE lock might need to block until all SIREAD locks on the
>> relation have been released. Picture, for example, what might
>> happen if one transaction acquires some predicate locks, then
>> commits (releasing its heavyweight lock on the table), and before
>> concurrent READ WRITE transactions complete there is a CLUSTER on
>> the table. Or a DROP INDEX. :-(
>
> Sorry, I can't picture it. What will happen?

Rather than get into a complex generalized discussion, I'll provide
the simplest example I can picture.

Let's say we have two concurrent transactions, T0 and T1. Up to
this point T0 has read from table x and written to table y based on
what was read from x. T1 has read from y -- but since the
transactions are concurrent, it doesn't see T0's write. Let's
assume each read was of a single tuple accessed through a btree
index, so each transaction has one tuple lock on the heap and one
page lock on the index. Now T0 commits. T0 must hold its SIREAD
locks because of concurrent transaction T1. Everything is fine so
far. Now a DBA runs CLUSTER against table x. The SIREAD locks held
by T0 are probably now wrong, because the tuple and its index entry
are likely to have moved. Now T1 writes to table x based on what it
read from y. It could incorrectly detect a conflict if it happens
to write to a tuple at the locked block and tuple number when it's
not the same row. Worse, it could miss detecting a conflict if it's
really updating the same row that T0 wrote, and that's not detected
because it's not at the locked location any more.

>> If this change is too destabilizing for this point in the release
>> we could document it as a limitation and fix it in 9.2.
>
> I don't think this should wait for 9.2
>
> It either works, or it doesn't. Putting caveats in there will just
> detract from people's belief in it.

I see your point. And this clearly is a bug. We failed to consider
this category of problem and cover it.

Heikki's suggestion is clearly the best plan. In the example above,
when the CLUSTER was run it would make a call to the predicate
locking module telling it to promote all SIREAD locks for table x or
any of its indexes into a relation level lock on table x. The
CLUSTER would cause us to lose the finer granularity of the locks on
the table, and in this example if T1 wrote to table x it be rolled
back with a serialization failure. This could be a false positive,
but we expect to have some of those -- the transaction is retried
and then succeeds. You can't have a false negative, so integrity is
preserved.

I'll try to work up a detailed plan of which commands need what
actions. For example, DROP INDEX needs to promote SIREAD locks on
the dropped index to relation locks on the related table. TRUNCATE
TABLE is a little confusing -- I think that if it's run in a
serializable transaction we generate a rw-conflict out from that
transaction to every transaction holding any SIREAD lock on that
table or any of its indexes, and then clear those SIREAD locks.
This'll take some study.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:

> This'll take some study.

I've gone through the list of commands in the development docs with
an eye toward exposing anything else we might have missed in dealing
with the SSI predicate locking. Some of this needs further
research, but I'm posting what I have so far so that people have a
chance to argue about what I think needs doing or to point out
anything else they can think of which I'm still missing. And, of
course, so people know where we are with it.

I tested file_fdw at the serializable isolation level and found that
no predicate locks were taken and no problems showed up. I think
that's all correct -- I don't see how we can expect to include FDW
data in serializability.

I haven't dug into ALTER INDEX enough to know whether it can ever
cause an index to be rebuilt. If so, we need to treat it like DROP
INDEX and REINDEX -- which should change all predicate locks of any
granularity on the index into relation locks on the associated
table.

CLUSTER or an ALTER TABLE which causes a rewrite should change all
predicate locks on the table and all indexes into relation locks on
the associated table. (Obviously, an existing relation lock on the
table doesn't require any action.)

TRUNCATE TABLE and DROP TABLE should generate a rw-conflict *in* to
the enclosing transaction (if it is serializable) from all
transactions holding predicate locks on the table or its indexes.
Note that this could cause a transactions which is running one of
these statements to roll back with a serialization error. This seems
correct to me, since these operations essentially delete all rows.
If you don't want the potential rollback, these operations should be
run at another isolation level. The difference between these two
statements is that I think that TRUNCATE TABLE should also move the
existing predicate locks to relation locks on the table while DROP
TABLE (for obvious reasons) should just delete the predicate locks.

DROP DATABASE should quietly clean up any predicate locks from
committed transactions which haven't yet hit their cleanup point
because of overlapping transactions in other databases.

DROP OWNED or DROP SCHEMA could CASCADE to some of the above, in
which case the above rules would apply.

I need to do some testing with Large Objects to see what state those
are in with SSI, unless someone else has already looked at that.

SAVEPOINTs have been considered and there is a lot of coding to try
to handle them correctly, but they probably merit a bit more
attention and testing. On some quick testing everything seemed to
be in line with previous discussions and with what seems logical to
me, but our SSI regression tests in src/test/isolation lack any
coverage for them and I don't know how much others have beat up on
them. At a minimum we should add a couple tests.

I couldn't find anything else which hadn't already been considered
and covered.

More to come as I finish investigating.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:

> I haven't dug into ALTER INDEX enough to know whether it can ever
> cause an index to be rebuilt. If so, we need to treat it like
> DROP INDEX and REINDEX -- which should change all predicate locks
> of any granularity on the index into relation locks on the
> associated table.
>
> CLUSTER or an ALTER TABLE which causes a rewrite should change all
> predicate locks on the table and all indexes into relation locks
> on the associated table. (Obviously, an existing relation lock on
> the table doesn't require any action.)
>
> TRUNCATE TABLE and DROP TABLE should generate a rw-conflict *in*
> to the enclosing transaction (if it is serializable) from all
> transactions holding predicate locks on the table or its indexes.
> Note that this could cause a transactions which is running one of
> these statements to roll back with a serialization error. This
> seems correct to me, since these operations essentially delete all
> rows. If you don't want the potential rollback, these operations
> should be run at another isolation level. The difference between
> these two statements is that I think that TRUNCATE TABLE should
> also move the existing predicate locks to relation locks on the
> table while DROP TABLE (for obvious reasons) should just delete
> the predicate locks.
>
> DROP DATABASE should quietly clean up any predicate locks from
> committed transactions which haven't yet hit their cleanup point
> because of overlapping transactions in other databases.

I missed VACUUM FULL when pulling together the above, but I haven't
found any other omissions. (Still happy to hear about any that
anyone can spot.)

I notice that most of these need to shift transfer locks to relation
locks on the heap, either for a single index or for the heap and all
indexes. I wrote a function to do this and called it from one place
to be able to test it. Consider this a WIP patch on which I would
appreciate review while I work on finding the other places to call
it and the miscellaneous other fixes needed.

Note that I had to expose one previously-static function from
index.c to find the heap OID from the index OID. Also, I ran
pgindent against predicate.c, as I generally like to do when I
modify much code, and it found four comment blocks in predicate.c
touched since the recent global pgindent run which it re-wrapped.
I can manually exclude those from the final patch if people would
prefer that; but if people can ignore those whitespace tweaks, it
might not be all bad to get standard formatting onto them at this
point.

-Kevin

Just a quick status update.

I wrote:

> Consider this a WIP patch

The serializable branch on my git repo has a modified form of this
which has been tested successfully with:

DROP INDEX
REINDEX
VACUUM FULL
CLUSTER
ALTER TABLE

I'm holding off on posting another version of the patch until I
cover the remaining commands, which are:

TRUNCATE TABLE
DROP TABLE
DROP DATABASE

I'm having to work on this off-hours at the moment, so expect the
patch to come sometime this weekend.

-Kevin

On 30.04.2011 01:04, Kevin Grittner wrote:
> TRUNCATE TABLE and DROP TABLE should generate a rw-conflict *in* to
> the enclosing transaction (if it is serializable) from all
> transactions holding predicate locks on the table or its indexes.
> Note that this could cause a transactions which is running one of
> these statements to roll back with a serialization error. This seems
> correct to me, since these operations essentially delete all rows.
> If you don't want the potential rollback, these operations should be
> run at another isolation level. The difference between these two
> statements is that I think that TRUNCATE TABLE should also move the
> existing predicate locks to relation locks on the table while DROP
> TABLE (for obvious reasons) should just delete the predicate locks.

Note that TRUNCATE has never been MVCC-safe anyway. Perhaps it's best to
just treat it like DROP TABLE. Or can we use the predicate lock
mechanism to abort serializable transactions that incorrectly see the
table as empty?

> DROP DATABASE should quietly clean up any predicate locks from
> committed transactions which haven't yet hit their cleanup point
> because of overlapping transactions in other databases.

This is just an optimization, right? The predicate locks will eventually
go away anyway.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 1 May 2011 I wrote:

> Consider this a WIP patch

Just so people know where this stands...

By 8 May 2011 I had the attached. I didn't post it because I was
not confident I had placed the calls to the SSI functions for DROP
TABLE and TRUNCATE TABLE correctly. Then came PGCon and also the
row versus tuple discussion -- the patch for which had conflicts
with this one.

I will be reviewing this on the weekend and making any final
adjustments, but the patch is very likely to look just like this
with the possible exception of placement of the DROP TABLE and
TRUNCATE TABLE calls. If anyone wants to start reviewing this now,
it would leave more time for me to make changes if needed.

Also, if anyone has comments or hints about the placement of those
calls, I'd be happy to receive them.

This patch compiles with `make world`, passes `make check-world`,
and passes both `make installcheck-world` and `make -C
src/test/isolation installcheck` against a database with
default_transaction_isolation = 'serializable'. It also passed a
few ad hoc tests of the implemented functionality.

-Kevin

On 03.06.2011 21:04, Kevin Grittner wrote:
> Also, if anyone has comments or hints about the placement of those
> calls, I'd be happy to receive them.

heap_drop_with_catalog() schedules the relation for deletion at the end
of transaction, but it's still possible that the transaction aborts and
the heap doesn't get dropped after all. If you put the
DropAllPredicateLocksFromTable() call there, and the transaction later
aborts, you've lost all the locks already.

I think you'll need to just memorize the lock deletion command in a
backend-local list, and perform the deletion in a post-commit function.
Something similar to the PendingRelDelete stuff in storage.c. In fact,
hooking into smgrDoPendingDeletes would work, but that seems like a
modularity violation.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> On 03.06.2011 21:04, Kevin Grittner wrote:
>> Also, if anyone has comments or hints about the placement of
>> those calls, I'd be happy to receive them.
>
> heap_drop_with_catalog() schedules the relation for deletion at
> the end of transaction, but it's still possible that the
> transaction aborts and the heap doesn't get dropped after all. If
> you put the DropAllPredicateLocksFromTable() call there, and the
> transaction later aborts, you've lost all the locks already.
>
> I think you'll need to just memorize the lock deletion command in
> a backend-local list, and perform the deletion in a post-commit
> function. Something similar to the PendingRelDelete stuff in
> storage.c. In fact, hooking into smgrDoPendingDeletes would work,
> but that seems like a modularity violation.

Thanks. That's helpful. Will look at that code and do something
similar.

I knew it didn't look right in the place it was, but couldn't quite
see what to do instead....

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> I think you'll need to just memorize the lock deletion command in
>> a backend-local list, and perform the deletion in a post-commit
>> function. Something similar to the PendingRelDelete stuff in
>> storage.c. In fact, hooking into smgrDoPendingDeletes would work,
>> but that seems like a modularity violation.

> Thanks. That's helpful. Will look at that code and do something
> similar.

Keep in mind that it's too late to throw any sort of error post-commit.
Any code you add there will need to have negligible probability of
failure.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>> I think you'll need to just memorize the lock deletion command
>>< in a backend-local list, and perform the deletion in a
>>> post-commit function. Something similar to the PendingRelDelete
>>> stuff in storage.c. In fact, hooking into smgrDoPendingDeletes
>>> would work, but that seems like a modularity violation.
>
>> Thanks. That's helpful. Will look at that code and do something
>> similar.
>
> Keep in mind that it's too late to throw any sort of error
> post-commit. Any code you add there will need to have negligible
> probability of failure.

Thanks for the heads-up. I think we're OK in that regard, though.
We have two commit-time functions in SSI:

PreCommit_CheckForSerializationFailure(void) which is the "last
chance for failure" before actual commit, and

ReleasePredicateLocks(const bool isCommit) which is for resource
cleanup after rollback or commit is effective.

We pretty much can't fail on the latter except for Assert
statements, and this new logic would be the same. It's hard to fail
when freeing resources unless something is quite seriously hosed
already. This is where I was planning to put the freeing of the
locks, based on queuing up the request at the time the DROP TABLE
statement is encountered.

-Kevin

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:

> I think you'll need to just memorize the lock deletion command in
> a backend-local list, and perform the deletion in a post-commit
> function.

Hmm. As mentioned earlier in the thread, cleaning these up doesn't
actually have any benefit beyond freeing space in the predicate
locking collections. I'm not sure that benefit is enough to justify
this much new mechanism. Maybe I should just leave them alone and
let them get cleaned up in due course with the rest of the locks.
Any opinions on that?

-Kevin

On 03.06.2011 23:44, Kevin Grittner wrote:
> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>
>> I think you'll need to just memorize the lock deletion command in
>> a backend-local list, and perform the deletion in a post-commit
>> function.
>
> Hmm. As mentioned earlier in the thread, cleaning these up doesn't
> actually have any benefit beyond freeing space in the predicate
> locking collections. I'm not sure that benefit is enough to justify
> this much new mechanism. Maybe I should just leave them alone and
> let them get cleaned up in due course with the rest of the locks.
> Any opinions on that?

Is there a chance of false positives if oid wraparound happens and a new
table gets the same oid as the old one? It's also possible for a heap to
get the OID of an old index or vice versa, will that confuse things?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> On 03.06.2011 23:44, Kevin Grittner wrote:

>> Hmm. As mentioned earlier in the thread, cleaning these up
>> doesn't actually have any benefit beyond freeing space in the
>> predicate locking collections. I'm not sure that benefit is
>> enough to justify this much new mechanism. Maybe I should just
>> leave them alone and let them get cleaned up in due course with
>> the rest of the locks. Any opinions on that?
>
> Is there a chance of false positives if oid wraparound happens and
> a new table gets the same oid as the old one? It's also possible
> for a heap to get the OID of an old index or vice versa, will that
> confuse things?

Tuple locks should be safe from that because we use the tuple xmin
as part of the target key, but page and heap locks could result in
false positive serialization failures on OID wraparound unless we do
the cleanup. I guess that tips the scales in favor of it being
worth the extra code. I think it's still in that gray area where
it's a judgment call because it would be very infrequent and it
would be recoverable; but the fewer mysterious rollbacks, the fewer
posts about it we'll get. So any costs in maintaining the extra
code will probably be saved in reduced support, even if the
performance benefit is negligible.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:

> Tuple locks should be safe from that because we use the tuple xmin
> as part of the target key, but page and heap locks

That should have read "page and relation locks".

> I guess that tips the scales in favor of it being worth the extra
> code. I think it's still in that gray area

I just thought of something which takes it out of the gray area for
me: pg_locks. Even though it would be extremely rare for a false
positive to actually occur if we let this go, people would be sure
to get confused by seeing locks on the dropped objects in the
pg_locks view.. They've got to be cleaned up.

-Kevin

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> On 03.06.2011 21:04, Kevin Grittner wrote:
>> Also, if anyone has comments or hints about the placement of those
>> calls, I'd be happy to receive them.

> heap_drop_with_catalog() schedules the relation for deletion at the end
> of transaction, but it's still possible that the transaction aborts and
> the heap doesn't get dropped after all. If you put the
> DropAllPredicateLocksFromTable() call there, and the transaction later
> aborts, you've lost all the locks already.

But on the third thought: is that wrong? Surely locks taken by an
aborted transaction can be discarded.

regards, tom lane

On 04.06.2011 19:19, Tom Lane wrote:
> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
>> On 03.06.2011 21:04, Kevin Grittner wrote:
>>> Also, if anyone has comments or hints about the placement of those
>>> calls, I'd be happy to receive them.
>
>> heap_drop_with_catalog() schedules the relation for deletion at the end
>> of transaction, but it's still possible that the transaction aborts and
>> the heap doesn't get dropped after all. If you put the
>> DropAllPredicateLocksFromTable() call there, and the transaction later
>> aborts, you've lost all the locks already.
>
> But on the third thought: is that wrong? Surely locks taken by an
> aborted transaction can be discarded.

These are predicate locks - there can be "locks" on the table belonging
to transactions that have already committed.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com