Thread: Strange locking choices in pg_shdepend.c
I came across some rather strange choices of lock levels in pg_shdepend.c.
Why does shdepDropOwned() take AccessExclusiveLock on pg_shdepend?
Seems like RowExclusiveLock should be sufficient. If it isn't
sufficient, I wonder whether the other functions in here are taking
strong enough locks.
It's probably not a good idea to have shdepReassignOwned() take only
AccessShareLock on pg_shdepend. Even though the function itself
merely reads the table, it is going to call functions that will take
RowExclusiveLock, meaning that we're setting ourselves up for potential
deadlock failures due to lock-upgrade. It'd be safer (and faster too)
to just hold RowExclusiveLock through the whole operation.
regards, tom lane
Tom Lane wrote: > I came across some rather strange choices of lock levels in pg_shdepend.c. > > Why does shdepDropOwned() take AccessExclusiveLock on pg_shdepend? > Seems like RowExclusiveLock should be sufficient. If it isn't > sufficient, I wonder whether the other functions in here are taking > strong enough locks. Hmm, I can't recall nor deduce any reason for that. Perhaps the intention was to protect against itself; but I think this should only matter if we're dropping the same role concurrently (otherwise the to-be-dropped objects would be disjoint sets, so it doesn't matter), which should be already protected by the lock on the role itself. Hmm, unless revoking privileges concurrently, for two different users on the same object could cause a problem? I don't see us grabbing a lock on the object itself -- does this matter? > It's probably not a good idea to have shdepReassignOwned() take only > AccessShareLock on pg_shdepend. Even though the function itself > merely reads the table, it is going to call functions that will take > RowExclusiveLock, meaning that we're setting ourselves up for potential > deadlock failures due to lock-upgrade. It'd be safer (and faster too) > to just hold RowExclusiveLock through the whole operation. Huh, correct. I think this was just an oversight. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> Why does shdepDropOwned() take AccessExclusiveLock on pg_shdepend?
> Hmm, I can't recall nor deduce any reason for that. Perhaps the
> intention was to protect against itself; but I think this should only
> matter if we're dropping the same role concurrently (otherwise the
> to-be-dropped objects would be disjoint sets, so it doesn't matter),
> which should be already protected by the lock on the role itself.
> Hmm, unless revoking privileges concurrently, for two different users on
> the same object could cause a problem? I don't see us grabbing a lock
> on the object itself -- does this matter?
Well, if there is any such problem then it could be triggered by two
independent plain-ol-REVOKE commands, so I still don't see an argument
why shdepDropOwned is more at risk than anything else. I think we
should just downgrade the lock.
regards, tom lane
Tom Lane wrote: > Well, if there is any such problem then it could be triggered by two > independent plain-ol-REVOKE commands, so I still don't see an argument > why shdepDropOwned is more at risk than anything else. I think we > should just downgrade the lock. Both issues fixed, thanks. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Hmm, unless revoking privileges concurrently, for two different users on > the same object could cause a problem? I don't see us grabbing a lock > on the object itself -- does this matter? I tried a simple test: a process in a loop calling GRANT and REVOKE on random users on a given table, and another process calling DROP OWNED BY another set of users. Prepare the test: psql -c "create table foo()" for i in `seq 0 100`; do psql -c "create user u$i"; done for i in `seq 0 100`; do psql -c "create user v$i"; done for i in `seq 0 100`; do psql -c "grant select on table foo to u$i"; done Then, on one terminal while true do r=$((RANDOM * 100 / 32764)) s=$((RANDOM * 100 / 32764)) psql -c "grant select on table foo to v$r" psql -c "revoke selecton table foo from v$s" done And another terminal for i in `seq 1 100`; do psql -c "drop owned by u$i"; done I get a lot of ERREUR: tuple concurrently updated So, yeah, I think our GRANT/REVOKE code has a race condition, which probably isn't very critical at all but it's still there. -- Alvaro Herrera Valdivia, Chile Geotag: -39,815 -73,257 "God is real, unless declared as int"
On Mon, Jan 21, 2008 at 04:54:06PM -0500, Tom Lane wrote: > It's probably not a good idea to have shdepReassignOwned() take only > AccessShareLock on pg_shdepend. Even though the function itself > merely reads the table, it is going to call functions that will take > RowExclusiveLock, meaning that we're setting ourselves up for potential > deadlock failures due to lock-upgrade. It'd be safer (and faster too) > to just hold RowExclusiveLock through the whole operation. Just a thought... Would it be worthwhile to allow for logging when a lock gets upgraded? That would make it easier to protect against deadlocks... -- Decibel!, aka Jim C. Nasby, Database Architect decibel@decibel.org Give your computer some brain candy! www.distributed.net Team #1828
Decibel! <decibel@decibel.org> writes:
> Would it be worthwhile to allow for logging when a lock gets upgraded?
> That would make it easier to protect against deadlocks...
There is some debug code for that in the backend, but my experience
is that it's too noisy to have on by default.
regards, tom lane
Tom Lane wrote: > Decibel! <decibel@decibel.org> writes: > > Would it be worthwhile to allow for logging when a lock gets upgraded? > > That would make it easier to protect against deadlocks... > > There is some debug code for that in the backend, but my experience > is that it's too noisy to have on by default. Seems a good idea to separate the upgrade bits from the other stuff and enable it on --enable-cassert or something similar. TODO for 8.4? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support