Re: pgsql: Add checks to TRUNCATE, CLUSTER, and REINDEX to prevent

Log Message:
-----------
Add checks to TRUNCATE, CLUSTER, and REINDEX to prevent performing these
operations when the current transaction has any open references to the
target relation or index (implying it has an active query using the relation).
The need for this was previously recognized in connection with ALTER TABLE,
but anything that summarily eliminates tuples or moves them around would
confuse an active scan.

While this patch does not in itself fix bug #3883 (the deadlock would happen
before the new check fires), it will discourage people from attempting the
sequence of operations that creates a deadlock risk, so it's at least a
partial response to that problem.

In passing, add a previously-missing check to REINDEX to prevent trying to
reindex another backend's temp table. This isn't a security problem since
only a superuser would get past the schema permission checks, but if we are
testing for this in other utility commands then surely REINDEX should too.

Modified Files:
--------------
pgsql/src/backend/catalog:
index.c (r1.291 -> r1.292)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/index.c?r1=1.291&r2=1.292)
pgsql/src/backend/commands:
cluster.c (r1.168 -> r1.169)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/cluster.c?r1=1.168&r2=1.169)
tablecmds.c (r1.240 -> r1.241)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/tablecmds.c?r1=1.240&r2=1.241)
pgsql/src/include/commands:
tablecmds.h (r1.36 -> r1.37)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/commands/tablecmds.h?r1=1.36&r2=1.37)

Tom Lane wrote:
> Log Message:
> -----------
> Add checks to TRUNCATE, CLUSTER, and REINDEX to prevent performing these
> operations when the current transaction has any open references to the
> target relation or index (implying it has an active query using the relation).
> The need for this was previously recognized in connection with ALTER TABLE,
> but anything that summarily eliminates tuples or moves them around would
> confuse an active scan.
>
> While this patch does not in itself fix bug #3883 (the deadlock would happen
> before the new check fires), it will discourage people from attempting the
> sequence of operations that creates a deadlock risk, so it's at least a
> partial response to that problem.
>
> In passing, add a previously-missing check to REINDEX to prevent trying to
> reindex another backend's temp table. This isn't a security problem since
> only a superuser would get past the schema permission checks, but if we are
> testing for this in other utility commands then surely REINDEX should too.

You want a TODO for 3883?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> You want a TODO for 3883?

Yeah, I would like to have some positive detection for deadlocks
involving LockBufferForCleanup(). What is unclear is how the heck
to do that without adding unacceptable overhead to buffer pin/unpin.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > You want a TODO for 3883?
>
> Yeah, I would like to have some positive detection for deadlocks
> involving LockBufferForCleanup(). What is unclear is how the heck
> to do that without adding unacceptable overhead to buffer pin/unpin.

OK, added to TODO:

* Improve deadlock detection when deleting items from shared buffers

http://archives.postgresql.org/pgsql-bugs/2008-01/msg00138.php
http://archives.postgresql.org/pgsql-hackers/2008-01/msg00873.php
http://archives.postgresql.org/pgsql-committers/2008-01/msg00365.php

Let me know if there is better wording.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

"Bruce Momjian" <bruce(at)momjian(dot)us> writes:

> * Improve deadlock detection when deleting items from shared buffers
>...
> Let me know if there is better wording.

I'm not sure whhere deleting items from shared buffers enters into it.

I think you need something like:

Add deadlock detection when a process holding a buffer pin is blocked by a
lock held by a process attempting to LockBufferForCleanup() on that buffer or
more complex versions thereof. (And without adding unacceptable overhead to
pin/unpin.)

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's On-Demand Production Tuning

Gregory Stark wrote:
> "Bruce Momjian" <bruce(at)momjian(dot)us> writes:
>
> > * Improve deadlock detection when deleting items from shared buffers
> >...
> > Let me know if there is better wording.
>
> I'm not sure whhere deleting items from shared buffers enters into it.
>
> I think you need something like:
>
> Add deadlock detection when a process holding a buffer pin is blocked by a
> lock held by a process attempting to LockBufferForCleanup() on that buffer or
> more complex versions thereof. (And without adding unacceptable overhead to
> pin/unpin.)

Text updated:

* Improve deadlock detection when a page cleaning lock conflicts
with a shared buffer that is pinned

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +