Strange locking choices in pg_shdepend.c

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 select on 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