On Wed, 2010-07-28 at 15:24 +0300, Peter Eisentraut wrote:
> On tor, 2010-07-15 at 10:24 +0100, Simon Riggs wrote:
> > Patch to reduce lock levels for
> > ALTER TABLE
> > CREATE TRIGGER
> > CREATE RULE
>
> Tried this out, but $subject is still the case. The problem is that
> ATRewriteCatalogs() calls AlterTableCreateToastTable() based on what it
> thinks the subcommands are, and AlterTableCreateToastTable() takes an
> AccessExclusiveLock.
>
> This could possibly be addressed by moving AT_SetStatistics to
> AT_PASS_MISC in order to avoid the TOAST table call.
>
> In a related matter, assigning ShareUpdateExclusiveLock to AT_SetStorage
> doesn't work either, because the TOAST table call needs to be done in
> that case.
>
> Perhaps this logic needs to be refactored a bit more so that there
> aren't any inconsistencies between the lock modes and the "passes",
> which could lead to false expectations and deadlocks.
Changes as suggested, plus tests to confirm lock levels for
ShareUpdateExclusiveLock changes. Will commit soon, if no objection.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 516dbd2..77a3c57 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -376,7 +376,7 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
/* Check heap and index are valid to cluster on */
if (OidIsValid(indexOid))
- check_index_is_clusterable(OldHeap, indexOid, recheck);
+ check_index_is_clusterable(OldHeap, indexOid, recheck, AccessExclusiveLock);
/* Log what we're doing (this could use more effort) */
if (OidIsValid(indexOid))
@@ -405,11 +405,11 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
* definition can't change under us.
*/
void
-check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck)
+check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMODE lockmode)
{
Relation OldIndex;
- OldIndex = index_open(indexOid, AccessExclusiveLock);
+ OldIndex = index_open(indexOid, lockmode);
/*
* Check that index is in fact an index on the given relation
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9a7cd96..defb2e4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2550,6 +2550,8 @@ AlterTableGetLockLevel(List *cmds)
case AT_DropCluster:
case AT_SetRelOptions:
case AT_ResetRelOptions:
+ case AT_SetOptions:
+ case AT_ResetOptions:
case AT_SetStorage:
cmd_lockmode = ShareUpdateExclusiveLock;
break;
@@ -2669,19 +2671,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* Performs own permission checks */
ATPrepSetStatistics(rel, cmd->name, cmd->def, lockmode);
- pass = AT_PASS_COL_ATTRS;
+ pass = AT_PASS_MISC;
break;
case AT_SetOptions: /* ALTER COLUMN SET ( options ) */
case AT_ResetOptions: /* ALTER COLUMN RESET ( options ) */
ATSimplePermissionsRelationOrIndex(rel);
/* This command never recurses */
- pass = AT_PASS_COL_ATTRS;
+ pass = AT_PASS_MISC;
break;
case AT_SetStorage: /* ALTER COLUMN SET STORAGE */
ATSimplePermissions(rel, false);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
- pass = AT_PASS_COL_ATTRS;
+ pass = AT_PASS_MISC;
break;
case AT_DropColumn: /* DROP COLUMN */
ATSimplePermissions(rel, false);
@@ -6906,7 +6908,7 @@ ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode)
indexName, RelationGetRelationName(rel))));
/* Check index is valid to cluster on */
- check_index_is_clusterable(rel, indexOid, false);
+ check_index_is_clusterable(rel, indexOid, false, lockmode);
/* And do the work */
mark_index_clustered(rel, indexOid);
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 4f67930..74d4bd1 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -14,6 +14,7 @@
#define CLUSTER_H
#include "nodes/parsenodes.h"
+#include "storage/lock.h"
#include "utils/relcache.h"
@@ -21,7 +22,7 @@ extern void cluster(ClusterStmt *stmt, bool isTopLevel);
extern void cluster_rel(Oid tableOid, Oid indexOid, bool recheck,
bool verbose, int freeze_min_age, int freeze_table_age);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
- bool recheck);
+ bool recheck, LOCKMODE lockmode);
extern void mark_index_clustered(Relation rel, Oid indexOid);
extern Oid make_new_heap(Oid OIDOldHeap, Oid NewTableSpace);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5aff44f..83e24fd 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1473,6 +1473,127 @@ select * from another;
drop table another;
--
+-- lock levels
+--
+drop type lockmodes;
+ERROR: type "lockmodes" does not exist
+create type lockmodes as enum (
+ 'AccessShareLock'
+,'RowShareLock'
+,'RowExclusiveLock'
+,'ShareUpdateExclusiveLock'
+,'ShareLock'
+,'ShareRowExclusiveLock'
+,'ExclusiveLock'
+,'AccessExclusiveLock'
+);
+drop view my_locks;
+ERROR: view "my_locks" does not exist
+create or replace view my_locks as
+select case when c.relname like 'pg_toast%' then 'pg_toast' else c.relname end, max(mode::lockmodes) as max_lockmode
+from pg_locks l join pg_class c on l.relation = c.oid
+where virtualtransaction = (
+ select virtualtransaction
+ from pg_locks
+ where transactionid = txid_current()::integer)
+and locktype = 'relation'
+and relnamespace != (select oid from pg_namespace where nspname = 'pg_catalog')
+and c.relname != 'my_locks'
+group by c.relname;
+create table alterlock (f1 int primary key, f2 text);
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "alterlock_pkey" for table "alterlock"
+-- share update exclusive
+begin; alter table alterlock alter column f2 set statistics 150;
+select * from my_locks order by 1;
+ relname | max_lockmode
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+(1 row)
+
+rollback;
+begin; alter table alterlock cluster on alterlock_pkey;
+select * from my_locks order by 1;
+ relname | max_lockmode
+----------------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+ alterlock_pkey | ShareUpdateExclusiveLock
+(2 rows)
+
+commit;
+begin; alter table alterlock set without cluster;
+select * from my_locks order by 1;
+ relname | max_lockmode
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+(1 row)
+
+commit;
+begin; alter table alterlock set (fillfactor = 100);
+select * from my_locks order by 1;
+ relname | max_lockmode
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+ pg_toast | ShareUpdateExclusiveLock
+(2 rows)
+
+commit;
+begin; alter table alterlock reset (fillfactor);
+select * from my_locks order by 1;
+ relname | max_lockmode
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+ pg_toast | ShareUpdateExclusiveLock
+(2 rows)
+
+commit;
+begin; alter table alterlock set (toast.autovacuum_enabled = off);
+select * from my_locks order by 1;
+ relname | max_lockmode
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+ pg_toast | ShareUpdateExclusiveLock
+(2 rows)
+
+commit;
+begin; alter table alterlock set (autovacuum_enabled = off);
+select * from my_locks order by 1;
+ relname | max_lockmode
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+ pg_toast | ShareUpdateExclusiveLock
+(2 rows)
+
+commit;
+begin; alter table alterlock alter column f2 set (n_distinct = 1);
+select * from my_locks order by 1;
+ relname | max_lockmode
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+(1 row)
+
+rollback;
+begin; alter table alterlock alter column f2 set storage extended;
+select * from my_locks order by 1;
+ relname | max_lockmode
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+(1 row)
+
+rollback;
+-- share row exclusive
+begin; alter table alterlock alter column f2 set default 'x';
+select * from my_locks order by 1;
+ relname | max_lockmode
+-----------+-----------------------
+ alterlock | ShareRowExclusiveLock
+(1 row)
+
+rollback;
+-- cleanup
+drop table alterlock;
+drop view my_locks;
+drop type lockmodes;
+--
-- alter function
--
create function test_strict(text) returns text as
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 82c2e4e..760670c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1091,6 +1091,83 @@ select * from another;
drop table another;
--
+-- lock levels
+--
+drop type lockmodes;
+create type lockmodes as enum (
+ 'AccessShareLock'
+,'RowShareLock'
+,'RowExclusiveLock'
+,'ShareUpdateExclusiveLock'
+,'ShareLock'
+,'ShareRowExclusiveLock'
+,'ExclusiveLock'
+,'AccessExclusiveLock'
+);
+
+drop view my_locks;
+create or replace view my_locks as
+select case when c.relname like 'pg_toast%' then 'pg_toast' else c.relname end, max(mode::lockmodes) as max_lockmode
+from pg_locks l join pg_class c on l.relation = c.oid
+where virtualtransaction = (
+ select virtualtransaction
+ from pg_locks
+ where transactionid = txid_current()::integer)
+and locktype = 'relation'
+and relnamespace != (select oid from pg_namespace where nspname = 'pg_catalog')
+and c.relname != 'my_locks'
+group by c.relname;
+
+create table alterlock (f1 int primary key, f2 text);
+
+-- share update exclusive
+begin; alter table alterlock alter column f2 set statistics 150;
+select * from my_locks order by 1;
+rollback;
+
+begin; alter table alterlock cluster on alterlock_pkey;
+select * from my_locks order by 1;
+commit;
+
+begin; alter table alterlock set without cluster;
+select * from my_locks order by 1;
+commit;
+
+begin; alter table alterlock set (fillfactor = 100);
+select * from my_locks order by 1;
+commit;
+
+begin; alter table alterlock reset (fillfactor);
+select * from my_locks order by 1;
+commit;
+
+begin; alter table alterlock set (toast.autovacuum_enabled = off);
+select * from my_locks order by 1;
+commit;
+
+begin; alter table alterlock set (autovacuum_enabled = off);
+select * from my_locks order by 1;
+commit;
+
+begin; alter table alterlock alter column f2 set (n_distinct = 1);
+select * from my_locks order by 1;
+rollback;
+
+begin; alter table alterlock alter column f2 set storage extended;
+select * from my_locks order by 1;
+rollback;
+
+-- share row exclusive
+begin; alter table alterlock alter column f2 set default 'x';
+select * from my_locks order by 1;
+rollback;
+
+-- cleanup
+drop table alterlock;
+drop view my_locks;
+drop type lockmodes;
+
+--
-- alter function
--
create function test_strict(text) returns text as
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
