Thread: Thoughts about bug #3883
Steven Flatt's report in this thread: http://archives.postgresql.org/pgsql-bugs/2008-01/msg00138.php exposes two more-or-less-independent flaws. One problem is that we allow operations like TRUNCATE on tables that are open in the current backend. This poses a risk of strange behavior, such as regression=# create table foo as select x from generate_series(1,1000) x; SELECT regression=# begin; BEGIN regression=# declare c cursor for select * from foo; DECLARE CURSOR regression=# fetch 10 from c;x ---- 1 2 3 4 5 6 7 8 910 (10 rows) regression=# truncate foo; TRUNCATE TABLE regression=# fetch 10 from c;x ----11121314151617181920 (10 rows) regression=# fetch all from c; ERROR: could not read block 1 of relation 1663/133283/156727: read only 0 of 8192 bytes It's not too consistent that we could still read rows from c until we needed to fetch the next page of the table. For more complex queries involving indexscans, I'm afraid the behavior could be even more bizarre. What I propose we do about this is put the same check into TRUNCATE, CLUSTER, and REINDEX that is already in ALTER TABLE, namely that we reject the command if the current transaction is already holding the table open. The issue Steven directly complained of is a potential for undetected deadlock via LockBufferForCleanup. Ordinarily, buffer-level locks don't pose a deadlock risk because we don't hold one while trying to acquire another (except in UPDATE, which uses an ordering rule to avoid the risk). The problem with LockBufferForCleanup is that it can be blocked by a mere pin, which another backend could well hold while trying to acquire a lock that will be blocked by VACUUM. There are a couple of migitating factors: first, patching TRUNCATE et al as suggested above will prevent the immediate case, and second, as of 8.3 this isn't a problem for autovacuum because of the facility for kicking autovacuum off a table if it's blocking someone else's lock request. Still, undetected deadlocks are unpleasant, so it'd be nice to have some way to recognize the situation if we do get into it. I have no idea about a reasonable way to do that though. Getting the heavyweight lock manager involved in buffer accesses seems right out on performance grounds. Comments, ideas? regards, tom lane
Tom Lane wrote: > What I propose we do about this is put the same check into TRUNCATE, > CLUSTER, and REINDEX that is already in ALTER TABLE, namely that we > reject the command if the current transaction is already holding > the table open. +1. > The issue Steven directly complained of is a potential for undetected > deadlock via LockBufferForCleanup. Ordinarily, buffer-level locks don't > pose a deadlock risk because we don't hold one while trying to acquire > another (except in UPDATE, which uses an ordering rule to avoid the > risk). The problem with LockBufferForCleanup is that it can be blocked > by a mere pin, which another backend could well hold while trying to > acquire a lock that will be blocked by VACUUM. Seems like a hard problem. I wonder if we can invoke the deadlock checker in LockBufferForCleanup. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
I wrote:
> The issue Steven directly complained of is a potential for undetected
> deadlock via LockBufferForCleanup. Ordinarily, buffer-level locks don't
> pose a deadlock risk because we don't hold one while trying to acquire
> another (except in UPDATE, which uses an ordering rule to avoid the
> risk). The problem with LockBufferForCleanup is that it can be blocked
> by a mere pin, which another backend could well hold while trying to
> acquire a lock that will be blocked by VACUUM.
> There are a couple of migitating factors: first, patching TRUNCATE et al
> as suggested above will prevent the immediate case, and second, as of
> 8.3 this isn't a problem for autovacuum because of the facility for
> kicking autovacuum off a table if it's blocking someone else's lock
> request.
I did some experimentation and was dismayed to find that that last
statement isn't true --- at least on HPUX, the attempted SIGINT doesn't
cause autovacuum to cancel. Some debugging and manual-reading led me to
the conclusion that a signal marked SA_RESTART is serviced but does not
cause semop() to return with EINTR; it just goes right back to waiting,
so we never get to the CHECK_FOR_INTERRUPTS call in PGSemaphoreLock.
Furthermore this appears to be the behavior expected by the Single Unix
Spec. Cancellation does work on Linux, whose man page for semop() says
semop() is never automatically restarted after being interrupted by a signal handler, regardless of the
settingof the SA_RESTART flag when establishing a signal handler.
But there is no such statement in the SUS. Mac OS X seems to behave
like Linux, though this seems directly contrary to what it says in
its man pages. That may mean that other BSDen do likewise.
If HPUX is the only platform that behaves like this, it might be that
we don't care enough to fix it, but I suspect that we might have a
problem on some other platforms too.
PGSemaphoreLock is called in three places: LWLockAcquire, which isn't
a problem because we don't want to accept cancel interrupts there
anyway; ProcWaitForSignal, which is the problem case; and ProcSleep
(ie, heavyweight-lock acquisition), which survives this case because it
sets lockAwaited, which is tested inside the interrupt service routine
via LockWaitCancel. The simplest fix seems to be to invent an
additional flag variable "signalAwaited" which is set/cleared by
ProcWaitForSignal and checked by LockWaitCancel. This would make
cancelling out of a ProcWaitForSignal call exactly analogous to
cancelling out of a heavyweight-lock acquisition.
My first inclination is to fix this in HEAD but not back branches,
because HEAD is the only branch where we really care that much about
autovacuum responding to SIGINT. On the other hand, the undetected
deadlock is real enough in back branches, and maybe we should back-patch
it so that DBAs can get a manually issued signal to work. Thoughts?
regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > The simplest fix seems to be to invent an additional flag variable > "signalAwaited" which is set/cleared by ProcWaitForSignal and checked by > LockWaitCancel. This would make cancelling out of a ProcWaitForSignal call > exactly analogous to cancelling out of a heavyweight-lock acquisition. Is that the flag that is an assertion that no cleanup is needed? Or is that something else? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support!
Gregory Stark <stark@enterprisedb.com> writes:
> "Tom Lane" <tgl@sss.pgh.pa.us> writes:
>> The simplest fix seems to be to invent an additional flag variable
>> "signalAwaited" which is set/cleared by ProcWaitForSignal and checked by
>> LockWaitCancel. This would make cancelling out of a ProcWaitForSignal call
>> exactly analogous to cancelling out of a heavyweight-lock acquisition.
> Is that the flag that is an assertion that no cleanup is needed? Or is that
> something else?
No, the problem is merely to get LockWaitCancel to return "true" so that
StatementCancelHandler will go ahead with the immediate interrupt. No
new cleanup is needed other than resetting the new flag.
regards, tom lane