Re: 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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> So I'm not seeing the point of risking unsafe behavior
> in LOCK TABLE.

Me neither.

...Robert

On Mon, May 11, 2009 at 21:18, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 12:10, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> On Mon, May 11, 2009 at 21:18, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 Tue, May 12, 2009 at 14:40, Alex Hunsaker <badalex(at)gmail(dot)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(at)gmail(dot)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