Обсуждение: DROP TABLE vs inheritance
There was just another complaint about something we've heard about before, namely that dropping a child table doesn't interact nicely with queries concurrently accessing the parent table: http://archives.postgresql.org/pgsql-bugs/2009-05/msg00113.php As I responded there, this isn't fixable by the obvious method of making DROP TABLE try to lock the parent too. On reflection though it seems that there is a reasonably simple solution: we could make find_inheritance_children() and find_all_inheritors() acquire lock on each child table as they scan pg_inherits, and do try_relation_open() or equivalent to see if the child still exists. If not, assume the table just got dropped and ignore the pg_inherits entry. This would require an API change to let the callers tell them what type of lock they intend to acquire on each table, but overall it shouldn't result in any visible change in query behavior in normal cases --- we're just acquiring relation locks a bit earlier than we did before. The only arguable downside I can see is that if pg_inherits happens to contain a corrupt row with a bad child OID, you'd never hear about it. But that doesn't seem like a big problem. Since 8.4 already contains a number of changes designed to make concurrent-DROP scenarios work more safely than before, I'm strongly tempted to sneak this change into 8.4. Thoughts? regards, tom lane
I wrote: > it seems that there is a reasonably simple solution: we could make > find_inheritance_children() and find_all_inheritors() acquire lock > on each child table as they scan pg_inherits, and do try_relation_open() > or equivalent to see if the child still exists. If not, assume the > table just got dropped and ignore the pg_inherits entry. I've committed changes along this line, but there was one place that I thought needed further discussion to decide whether to change it. That is LockTableCommand(), which has historically attempted to determine whether the user has privilege on a table before it locks it. It's still working that way, which means it's at risk of the same type of child-disappeared problem that I just fixed elsewhere. I know we've gone back and forth on the question of how LOCK TABLE should behave, but at the moment I'm leaning towards changing it. The argument for the way it behaves now seems to be that a user who has no privileges on a table could cause a momentary denial of service to those who do by executing LOCK TABLE with an exclusive lock level. However, he can do that anyway via ALTER TABLE, which will happily take out AccessExclusiveLock before it checks any permissions. So I'm not seeing the point of risking unsafe behavior in LOCK TABLE. Comments? regards, tom lane
On Mon, May 11, 2009 at 11:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > So I'm not seeing the point of risking unsafe behavior > in LOCK TABLE. Me neither. ...Robert
On Tue, May 12, 2009 at 12:10, Alex Hunsaker <badalex@gmail.com> wrote: > On Mon, May 11, 2009 at 21:18, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> However, he can do that anyway via ALTER TABLE, which >> will happily take out AccessExclusiveLock before it checks any >> permissions. So I'm not seeing the point of risking unsafe behavior >> in LOCK TABLE. > > I would rather fix ALTER TABLE to do something similar to test and > test-and-set... From a quick look TRUNCATE also seems to be prone to > this. Arg ok so TRUNCATE was a bad example because it checks ACL_TRUNCATE. Hrm on second thought I think your right. They only get the lock until the permission check, and I have a hard time seeing how someone can take real advantage of that. The owner that is trying to lock table should get the lock almost immediately even if there are say a few hundred non-owner clients trying to lock it. So +1 for fixing the LOCK TABLE. Is ALTER TABLE RENAME at risk at well? It calls CheckRelationOwnership before it grabs an AccessExclusiveLock.
On Mon, May 11, 2009 at 21:18, Tom Lane <tgl@sss.pgh.pa.us> wrote: > However, he can do that anyway via ALTER TABLE, which > will happily take out AccessExclusiveLock before it checks any > permissions. So I'm not seeing the point of risking unsafe behavior > in LOCK TABLE. I would rather fix ALTER TABLE to do something similar to test and test-and-set... From a quick look TRUNCATE also seems to be prone to this.
On Tue, May 12, 2009 at 14:40, Alex Hunsaker <badalex@gmail.com> wrote: > Hrm on second thought I think your right. They only get the lock > until the permission check, and I have a hard time seeing how someone > can take real advantage of that. The owner that is trying to lock > table should get the lock almost immediately even if there are say a > few hundred non-owner clients trying to lock it. FWIW i just tested this with ~100 clients doing begin; ALTER TABLE test_lock ADD COLUMN commit; here is the timing. Is there some other concern that im not seeing? (pre 100 clients) => LOCK table test_lock; LOCK TABLE Time: 1.955 ms (now with 100 non-owner clients trying to do ALTER TABLE) => LOCK TABLE test_lock; LOCK TABLE Time: 71.746 ms *shrugs*
Alex Hunsaker <badalex@gmail.com> writes: > FWIW i just tested this with ~100 clients doing begin; ALTER TABLE > test_lock ADD COLUMN commit; here is the timing. Is there some other > concern that im not seeing? The situation where someone quickly acquires the lock isn't much of an issue, because they'll drop it almost immediately after the permissions check fails. However consider a scenario like this: 1. Legitimate user U1 does a SELECT on table T and then goes to sleep with open transaction. 2. Nefarious user U2 does LOCK TABLE T (exclusively). He blocks behind U1's transaction, since the permissions check won't happen till he gets the lock. 3. Now, everybody else trying to use table T will queue up behind U2's request. Their operations might have been perfectly able to run in parallel with U1's AccessShareLock, but they'll wait behind an ungranted AccessExclusiveLock. So it's definitely not a purely academic concern. However, there isn't any part of this behavior that we can change without breaking other stuff :-( regards, tom lane