Thread: Strange locking choices in pg_shdepend.c

Strange locking choices in pg_shdepend.c

From
Tom Lane
Date:
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


Re: Strange locking choices in pg_shdepend.c

From
Alvaro Herrera
Date:
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.


Re: Strange locking choices in pg_shdepend.c

From
Tom Lane
Date:
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


Re: Strange locking choices in pg_shdepend.c

From
Alvaro Herrera
Date:
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.


Re: Strange locking choices in pg_shdepend.c

From
Alvaro Herrera
Date:
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"


Re: Strange locking choices in pg_shdepend.c

From
Decibel!
Date:
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

Re: Strange locking choices in pg_shdepend.c

From
Tom Lane
Date:
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


Re: Strange locking choices in pg_shdepend.c

From
Alvaro Herrera
Date:
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