On Thu, 2010-07-08 at 07:16 +0100, Simon Riggs wrote: > I'll take my previous patch through to completion now
Patch to reduce lock levels for ALTER TABLE CREATE TRIGGER CREATE RULE I've completely re-analyzed the required lock levels for sub-commands, so lock levels can now also be these, if appropriate. ShareUpdateExclusiveLock - allows db reads and writes ShareRowExclusiveLock - allows db reads only When ALTER TABLE is specified with multiple subcommands the highest lock level required by any subcommand is applied to the whole combined command. The lock levels are in many ways different from both my own earlier patch and much of the discussion on this thread, which I have taken to be general comments rather than considered thought. Nothing much speculative here, so will commit in a few days barring objections. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Training and Services
*** a/doc/src/sgml/mvcc.sgml --- b/doc/src/sgml/mvcc.sgml *************** *** 532,538 **** SELECT SUM(value) FROM mytab WHERE class = 2; most <productname>PostgreSQL</productname> commands automatically acquire locks of appropriate modes to ensure that referenced tables are not dropped or modified in incompatible ways while the ! command executes. (For example, <command>ALTER TABLE</> cannot safely be executed concurrently with other operations on the same table, so it obtains an exclusive lock on the table to enforce that.) </para> --- 532,538 ---- most <productname>PostgreSQL</productname> commands automatically acquire locks of appropriate modes to ensure that referenced tables are not dropped or modified in incompatible ways while the ! command executes. (For example, <command>TRUNCATE</> cannot safely be executed concurrently with other operations on the same table, so it obtains an exclusive lock on the table to enforce that.) </para> *************** *** 695,702 **** SELECT SUM(value) FROM mytab WHERE class = 2; </para> <para> ! This lock mode is not automatically acquired by any ! <productname>PostgreSQL</productname> command. </para> </listitem> </varlistentry> --- 695,703 ---- </para> <para> ! Acquired by <command>CREATE TRIGGER</command>, ! <command>CREATE RULE</command> (except for <literal>ON SELECT</> ! rules) and in some cases <command>ALTER TABLE</command>. </para> </listitem> </varlistentry> *************** *** 742,752 **** SELECT SUM(value) FROM mytab WHERE class = 2; </para> <para> ! Acquired by the <command>ALTER TABLE</command>, <command>DROP ! TABLE</command>, <command>TRUNCATE</command>, <command>REINDEX</command>, <command>CLUSTER</command>, and <command>VACUUM FULL</command> ! commands. This is also the default lock mode for <command>LOCK ! TABLE</command> statements that do not specify a mode explicitly. </para> </listitem> </varlistentry> --- 743,754 ---- </para> <para> ! Acquired by the <command>DROP TABLE</command>, ! <command>TRUNCATE</command>, <command>REINDEX</command>, <command>CLUSTER</command>, and <command>VACUUM FULL</command> ! commands, as well as most variants of <command>ALTER TABLE</>. ! This is also the default lock mode for <command>LOCK TABLE</command> ! statements that do not specify a mode explicitly. </para> </listitem> </varlistentry> *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *************** *** 64,69 **** --- 64,70 ---- #include "rewrite/rewriteHandler.h" #include "storage/bufmgr.h" #include "storage/lmgr.h" + #include "storage/lock.h" #include "storage/smgr.h" #include "utils/acl.h" #include "utils/builtins.h" *************** *** 253,273 **** static void validateForeignKeyConstraint(Constraint *fkconstraint, Oid pkindOid, Oid constraintOid); static void createForeignKeyTriggers(Relation rel, Constraint *fkconstraint, Oid constraintOid, Oid indexOid); ! static void ATController(Relation rel, List *cmds, bool recurse); static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, ! bool recurse, bool recursing); ! static void ATRewriteCatalogs(List **wqueue); static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, ! AlterTableCmd *cmd); static void ATRewriteTables(List **wqueue); static void ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap); static AlteredTableInfo *ATGetQueueEntry(List **wqueue, Relation rel); static void ATSimplePermissions(Relation rel, bool allowView); static void ATSimplePermissionsRelationOrIndex(Relation rel); static void ATSimpleRecursion(List **wqueue, Relation rel, ! AlterTableCmd *cmd, bool recurse); static void ATOneLevelRecursion(List **wqueue, Relation rel, ! AlterTableCmd *cmd); static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, AlterTableCmd *cmd); static void ATExecAddColumn(AlteredTableInfo *tab, Relation rel, --- 254,274 ---- Oid pkindOid, Oid constraintOid); static void createForeignKeyTriggers(Relation rel, Constraint *fkconstraint, Oid constraintOid, Oid indexOid); ! static void ATController(Relation rel, List *cmds, bool recurse, LOCKMODE lockmode); static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, ! bool recurse, bool recursing, LOCKMODE lockmode); ! static void ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode); static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, ! AlterTableCmd *cmd, LOCKMODE lockmode); static void ATRewriteTables(List **wqueue); static void ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap); static AlteredTableInfo *ATGetQueueEntry(List **wqueue, Relation rel); static void ATSimplePermissions(Relation rel, bool allowView); static void ATSimplePermissionsRelationOrIndex(Relation rel); static void ATSimpleRecursion(List **wqueue, Relation rel, ! AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode); static void ATOneLevelRecursion(List **wqueue, Relation rel, ! AlterTableCmd *cmd, LOCKMODE lockmode); static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, AlterTableCmd *cmd); static void ATExecAddColumn(AlteredTableInfo *tab, Relation rel, *************** *** 293,308 **** static void ATExecDropColumn(List **wqueue, Relation rel, const char *colName, bool recurse, bool recursing, bool missing_ok); static void ATExecAddIndex(AlteredTableInfo *tab, Relation rel, ! IndexStmt *stmt, bool is_rebuild); static void ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, ! Constraint *newConstraint, bool recurse); static void ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, Constraint *constr, ! bool recurse, bool recursing); static void ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, ! Constraint *fkconstraint); static void ATExecDropConstraint(Relation rel, const char *constrName, DropBehavior behavior, bool recurse, bool recursing, --- 294,309 ---- bool recurse, bool recursing, bool missing_ok); static void ATExecAddIndex(AlteredTableInfo *tab, Relation rel, ! IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode); static void ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, ! Constraint *newConstraint, bool recurse, LOCKMODE lockmode); static void ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, Constraint *constr, ! bool recurse, bool recursing, LOCKMODE lockmode); static void ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, ! Constraint *fkconstraint, LOCKMODE lockmode); static void ATExecDropConstraint(Relation rel, const char *constrName, DropBehavior behavior, bool recurse, bool recursing, *************** *** 310,316 **** static void ATExecDropConstraint(Relation rel, const char *constrName, static void ATPrepAlterColumnType(List **wqueue, AlteredTableInfo *tab, Relation rel, bool recurse, bool recursing, ! AlterTableCmd *cmd); static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, const char *colName, TypeName *typeName); static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab); --- 311,317 ---- static void ATPrepAlterColumnType(List **wqueue, AlteredTableInfo *tab, Relation rel, bool recurse, bool recursing, ! AlterTableCmd *cmd, LOCKMODE lockmode); static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, const char *colName, TypeName *typeName); static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab); *************** *** 2237,2244 **** RenameRelationInternal(Oid myrelid, const char *newrelname, Oid namespaceId) * Disallow ALTER TABLE (and similar commands) when the current backend has * any open reference to the target table besides the one just acquired by * the calling command; this implies there's an open cursor or active plan. ! * We need this check because our AccessExclusiveLock doesn't protect us ! * against stomping on our own foot, only other people's feet! * * For ALTER TABLE, the only case known to cause serious trouble is ALTER * COLUMN TYPE, and some changes are obviously pretty benign, so this could --- 2238,2245 ---- * Disallow ALTER TABLE (and similar commands) when the current backend has * any open reference to the target table besides the one just acquired by * the calling command; this implies there's an open cursor or active plan. ! * We need this check because our lock doesn't protect us against stomping ! * on our own foot, only other people's feet! * * For ALTER TABLE, the only case known to cause serious trouble is ALTER * COLUMN TYPE, and some changes are obviously pretty benign, so this could *************** *** 2315,2325 **** CheckTableNotInUse(Relation rel, const char *stmt) * * Thanks to the magic of MVCC, an error anywhere along the way rolls back * the whole operation; we don't have to do anything special to clean up. */ void AlterTable(AlterTableStmt *stmt) { ! Relation rel = relation_openrv(stmt->relation, AccessExclusiveLock); CheckTableNotInUse(rel, "ALTER TABLE"); --- 2316,2342 ---- * * Thanks to the magic of MVCC, an error anywhere along the way rolls back * the whole operation; we don't have to do anything special to clean up. + * + * We lock the table as the first action, with an appropriate lock level + * for the subcommands requested. Any subcommand that needs to rewrite + * tuples in the table forces the whole command to be executed with + * AccessExclusiveLock. If all subcommands do not require rewrite table + * then we may be able to use lower lock levels. We pass the lock level down + * so that we can apply it recursively to inherited tables. Note that the + * lock level we want as we recurse may well be higher than required for + * that specific subcommand. So we pass down the overall lock requirement, + * rather than reassess it at lower levels. */ void AlterTable(AlterTableStmt *stmt) { ! Relation rel; ! LOCKMODE lockmode = AlterTableGreatestLockLevel(stmt->cmds); ! ! /* ! * Acquire same level of lock as already acquired during parsing. ! */ ! rel = relation_openrv(stmt->relation, lockmode); CheckTableNotInUse(rel, "ALTER TABLE"); *************** *** 2362,2368 **** AlterTable(AlterTableStmt *stmt) elog(ERROR, "unrecognized object type: %d", (int) stmt->relkind); } ! ATController(rel, stmt->cmds, interpretInhOption(stmt->relation->inhOpt)); } /* --- 2379,2386 ---- elog(ERROR, "unrecognized object type: %d", (int) stmt->relkind); } ! ATController(rel, stmt->cmds, interpretInhOption(stmt->relation->inhOpt), ! lockmode); } /* *************** *** 2379,2391 **** AlterTable(AlterTableStmt *stmt) void AlterTableInternal(Oid relid, List *cmds, bool recurse) { ! Relation rel = relation_open(relid, AccessExclusiveLock); ! ATController(rel, cmds, recurse); } static void ! ATController(Relation rel, List *cmds, bool recurse) { List *wqueue = NIL; ListCell *lcmd; --- 2397,2559 ---- void AlterTableInternal(Oid relid, List *cmds, bool recurse) { ! Relation rel; ! LOCKMODE lockmode = AlterTableGreatestLockLevel(cmds); ! ! rel = relation_open(relid, lockmode); ! ! ATController(rel, cmds, recurse, lockmode); ! } ! ! /* ! * AlterTableGreatestLockLevel ! * ! * Sets the overall lock level required for the supplied list of subcommands. ! * Policy for doing this set according to needs of AlterTable(), see ! * comments there for overall explanation. ! * ! * Function is called before and after parsing, so it must give same ! * answer each time it is called. Some subcommands are transformed ! * into other subcommand types, so the transform must never be made to a ! * lower lock level than previously assigned. All transforms are noted below. ! * ! * Since this is called before we lock the table we cannot use table metadata ! * to influence the type of lock we acquire. ! */ ! LOCKMODE ! AlterTableGreatestLockLevel(List *cmds) ! { ! ListCell *lcmd; ! LOCKMODE lockmode = ShareUpdateExclusiveLock; /* default for compiler */ ! ! foreach(lcmd, cmds) ! { ! AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd); ! LOCKMODE cmd_lockmode = AccessExclusiveLock; /* default for compiler */ ! ! switch (cmd->subtype) ! { ! /* ! * Need AccessExclusiveLock for these subcommands because they ! * affect or potentially affect both read and write operations. ! * ! * New subcommand types should be added here by default. ! */ ! case AT_AddColumn: /* may rewrite heap, in some cases and visible to SELECT */ ! case AT_DropColumn: /* change visible to SELECT */ ! case AT_AddColumnToView: /* CREATE VIEW */ ! case AT_AlterColumnType: /* must rewrite heap */ ! case AT_DropConstraint: /* as DROP INDEX */ ! case AT_AddOids: ! case AT_DropOids: /* calls AT_DropColumn */ ! case AT_EnableAlwaysRule: /* as DROP INDEX */ ! case AT_EnableReplicaRule: /* as DROP INDEX */ ! case AT_EnableRule: /* as DROP INDEX */ ! case AT_DisableRule: /* as DROP INDEX */ ! case AT_ChangeOwner: /* change visible to SELECT */ ! case AT_SetTableSpace: /* must rewrite heap */ ! case AT_DropNotNull: /* may change some SQL plans */ ! case AT_SetNotNull: ! cmd_lockmode = AccessExclusiveLock; ! break; ! ! /* ! * These subcommands affect write operations only. ! */ ! case AT_ColumnDefault: ! case AT_ProcessedConstraint: /* becomes AT_AddConstraint */ ! case AT_AddConstraintRecurse: /* becomes AT_AddConstraint */ ! case AT_EnableTrig: ! case AT_EnableAlwaysTrig: ! case AT_EnableReplicaTrig: ! case AT_EnableTrigAll: ! case AT_EnableTrigUser: ! case AT_DisableTrig: ! case AT_DisableTrigAll: ! case AT_DisableTrigUser: ! case AT_AddIndex: /* from ADD CONSTRAINT */ ! cmd_lockmode = ShareRowExclusiveLock; ! break; ! ! case AT_AddConstraint: ! if (IsA(cmd->def, Constraint)) ! { ! Constraint *con = (Constraint *) cmd->def; ! ! switch (con->contype) ! { ! case CONSTR_EXCLUSION: ! case CONSTR_PRIMARY: ! case CONSTR_UNIQUE: ! /* ! * Cases essentially the same as CREATE INDEX. We ! * could reduce the lock strength to ShareLock if we ! * can work out how to allow concurrent catalog updates. ! */ ! cmd_lockmode = ShareRowExclusiveLock; ! break; ! case CONSTR_FOREIGN: ! /* ! * We add triggers to both tables when we add a ! * Foreign Key, so the lock level must be at least ! * as strong as CREATE TRIGGER. ! */ ! cmd_lockmode = ShareRowExclusiveLock; ! break; ! ! default: ! cmd_lockmode = ShareRowExclusiveLock; ! } ! } ! break; ! ! /* ! * These subcommands affect inheritance behaviour. Queries started before us ! * will continue to see the old inheritance behaviour, while queries started ! * after we commit will see new behaviour. No need to prevent reads or writes ! * to the subtable while we hook it up though. In both cases the parent table ! * is locked with AccessShareLock. ! */ ! case AT_AddInherit: ! case AT_DropInherit: ! cmd_lockmode = ShareUpdateExclusiveLock; ! break; ! ! /* ! * These subcommands affect general strategies for performance and maintenance, ! * though don't change the semantic results from normal data reads and writes. ! * Delaying an ALTER TABLE behind currently active writes only delays the point ! * where the new strategy begins to take effect, so there is no benefit in waiting. ! * In thise case the minimum restriction applies: we don't currently allow ! * concurrent catalog updates. ! */ ! case AT_SetStatistics: ! case AT_ClusterOn: ! case AT_DropCluster: ! case AT_SetRelOptions: ! case AT_ResetRelOptions: ! case AT_SetStorage: ! cmd_lockmode = ShareUpdateExclusiveLock; ! break; ! ! default: /* oops */ ! elog(ERROR, "unrecognized alter table type: %d", ! (int) cmd->subtype); ! break; ! } ! ! /* ! * Take the greatest lockmode from any subcommand ! */ ! if (cmd_lockmode > lockmode) ! lockmode = cmd_lockmode; ! } ! return lockmode; } static void ! ATController(Relation rel, List *cmds, bool recurse, LOCKMODE lockmode) { List *wqueue = NIL; ListCell *lcmd; *************** *** 2395,2408 **** ATController(Relation rel, List *cmds, bool recurse) { AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd); ! ATPrepCmd(&wqueue, rel, cmd, recurse, false); } /* Close the relation, but keep lock until commit */ relation_close(rel, NoLock); /* Phase 2: update system catalogs */ ! ATRewriteCatalogs(&wqueue); /* Phase 3: scan/rewrite tables as needed */ ATRewriteTables(&wqueue); --- 2563,2576 ---- { AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd); ! ATPrepCmd(&wqueue, rel, cmd, recurse, false, lockmode); } /* Close the relation, but keep lock until commit */ relation_close(rel, NoLock); /* Phase 2: update system catalogs */ ! ATRewriteCatalogs(&wqueue, lockmode); /* Phase 3: scan/rewrite tables as needed */ ATRewriteTables(&wqueue); *************** *** 2414,2425 **** ATController(Relation rel, List *cmds, bool recurse) * Traffic cop for ALTER TABLE Phase 1 operations, including simple * recursion and permission checks. * ! * Caller must have acquired AccessExclusiveLock on relation already. * This lock should be held until commit. */ static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, ! bool recurse, bool recursing) { AlteredTableInfo *tab; int pass; --- 2582,2593 ---- * Traffic cop for ALTER TABLE Phase 1 operations, including simple * recursion and permission checks. * ! * Caller must have acquired appropriate lock type on relation already. * This lock should be held until commit. */ static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, ! bool recurse, bool recursing, LOCKMODE lockmode) { AlteredTableInfo *tab; int pass; *************** *** 2463,2486 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, * rules. */ ATSimplePermissions(rel, true); ! ATSimpleRecursion(wqueue, rel, cmd, recurse); /* No command-specific prep needed */ pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP; break; case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */ ATSimplePermissions(rel, false); ! ATSimpleRecursion(wqueue, rel, cmd, recurse); /* No command-specific prep needed */ pass = AT_PASS_DROP; break; case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */ ATSimplePermissions(rel, false); ! ATSimpleRecursion(wqueue, rel, cmd, recurse); /* No command-specific prep needed */ pass = AT_PASS_ADD_CONSTR; break; case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */ ! ATSimpleRecursion(wqueue, rel, cmd, recurse); /* Performs own permission checks */ ATPrepSetStatistics(rel, cmd->name, cmd->def); pass = AT_PASS_COL_ATTRS; --- 2631,2654 ---- * rules. */ ATSimplePermissions(rel, true); ! ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode); /* No command-specific prep needed */ pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP; break; case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */ ATSimplePermissions(rel, false); ! ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode); /* No command-specific prep needed */ pass = AT_PASS_DROP; break; case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */ ATSimplePermissions(rel, false); ! ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode); /* No command-specific prep needed */ pass = AT_PASS_ADD_CONSTR; break; case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */ ! ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode); /* Performs own permission checks */ ATPrepSetStatistics(rel, cmd->name, cmd->def); pass = AT_PASS_COL_ATTRS; *************** *** 2493,2499 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, break; case AT_SetStorage: /* ALTER COLUMN SET STORAGE */ ATSimplePermissions(rel, false); ! ATSimpleRecursion(wqueue, rel, cmd, recurse); /* No command-specific prep needed */ pass = AT_PASS_COL_ATTRS; break; --- 2661,2667 ---- 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; break; *************** *** 2530,2536 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, case AT_AlterColumnType: /* ALTER COLUMN TYPE */ ATSimplePermissions(rel, false); /* Performs own recursion */ ! ATPrepAlterColumnType(wqueue, tab, rel, recurse, recursing, cmd); pass = AT_PASS_ALTER_TYPE; break; case AT_ChangeOwner: /* ALTER OWNER */ --- 2698,2704 ---- case AT_AlterColumnType: /* ALTER COLUMN TYPE */ ATSimplePermissions(rel, false); /* Performs own recursion */ ! ATPrepAlterColumnType(wqueue, tab, rel, recurse, recursing, cmd, lockmode); pass = AT_PASS_ALTER_TYPE; break; case AT_ChangeOwner: /* ALTER OWNER */ *************** *** 2562,2568 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, dropCmd->subtype = AT_DropColumn; dropCmd->name = pstrdup("oid"); dropCmd->behavior = cmd->behavior; ! ATPrepCmd(wqueue, rel, dropCmd, recurse, false); } pass = AT_PASS_DROP; break; --- 2730,2736 ---- dropCmd->subtype = AT_DropColumn; dropCmd->name = pstrdup("oid"); dropCmd->behavior = cmd->behavior; ! ATPrepCmd(wqueue, rel, dropCmd, recurse, false, lockmode); } pass = AT_PASS_DROP; break; *************** *** 2617,2623 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, * conflicts). */ static void ! ATRewriteCatalogs(List **wqueue) { int pass; ListCell *ltab; --- 2785,2791 ---- * conflicts). */ static void ! ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode) { int pass; ListCell *ltab; *************** *** 2643,2654 **** ATRewriteCatalogs(List **wqueue) continue; /* ! * Exclusive lock was obtained by phase 1, needn't get it again */ rel = relation_open(tab->relid, NoLock); foreach(lcmd, subcmds) ! ATExecCmd(wqueue, tab, rel, (AlterTableCmd *) lfirst(lcmd)); /* * After the ALTER TYPE pass, do cleanup work (this is not done in --- 2811,2822 ---- continue; /* ! * Appropriate lock was obtained by phase 1, needn't get it again */ rel = relation_open(tab->relid, NoLock); foreach(lcmd, subcmds) ! ATExecCmd(wqueue, tab, rel, (AlterTableCmd *) lfirst(lcmd), lockmode); /* * After the ALTER TYPE pass, do cleanup work (this is not done in *************** *** 2683,2689 **** ATRewriteCatalogs(List **wqueue) */ static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, ! AlterTableCmd *cmd) { switch (cmd->subtype) { --- 2851,2857 ---- */ static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, ! AlterTableCmd *cmd, LOCKMODE lockmode) { switch (cmd->subtype) { *************** *** 2722,2739 **** ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, cmd->behavior, true, false, cmd->missing_ok); break; case AT_AddIndex: /* ADD INDEX */ ! ATExecAddIndex(tab, rel, (IndexStmt *) cmd->def, false); break; case AT_ReAddIndex: /* ADD INDEX */ ! ATExecAddIndex(tab, rel, (IndexStmt *) cmd->def, true); break; case AT_AddConstraint: /* ADD CONSTRAINT */ ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def, ! false); break; case AT_AddConstraintRecurse: /* ADD CONSTRAINT with recursion */ ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def, ! true); break; case AT_DropConstraint: /* DROP CONSTRAINT */ ATExecDropConstraint(rel, cmd->name, cmd->behavior, --- 2890,2907 ---- cmd->behavior, true, false, cmd->missing_ok); break; case AT_AddIndex: /* ADD INDEX */ ! ATExecAddIndex(tab, rel, (IndexStmt *) cmd->def, false, lockmode); break; case AT_ReAddIndex: /* ADD INDEX */ ! ATExecAddIndex(tab, rel, (IndexStmt *) cmd->def, true, lockmode); break; case AT_AddConstraint: /* ADD CONSTRAINT */ ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def, ! false, lockmode); break; case AT_AddConstraintRecurse: /* ADD CONSTRAINT with recursion */ ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def, ! true, lockmode); break; case AT_DropConstraint: /* DROP CONSTRAINT */ ATExecDropConstraint(rel, cmd->name, cmd->behavior, *************** *** 2971,2982 **** ATRewriteTables(List **wqueue) Relation refrel; if (rel == NULL) - { /* Long since locked, no need for another */ rel = heap_open(tab->relid, NoLock); - } ! refrel = heap_open(con->refrelid, RowShareLock); validateForeignKeyConstraint(fkconstraint, rel, refrel, con->refindid, --- 3139,3152 ---- Relation refrel; if (rel == NULL) /* Long since locked, no need for another */ rel = heap_open(tab->relid, NoLock); ! /* ! * We're adding a trigger to both tables, so the lock level ! * here should sensibly reflect that. ! */ ! refrel = heap_open(con->refrelid, ShareRowExclusiveLock); validateForeignKeyConstraint(fkconstraint, rel, refrel, con->refindid, *************** *** 3387,3393 **** ATSimplePermissionsRelationOrIndex(Relation rel) */ static void ATSimpleRecursion(List **wqueue, Relation rel, ! AlterTableCmd *cmd, bool recurse) { /* * Propagate to children if desired. Non-table relations never have --- 3557,3563 ---- */ static void ATSimpleRecursion(List **wqueue, Relation rel, ! AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode) { /* * Propagate to children if desired. Non-table relations never have *************** *** 3399,3405 **** ATSimpleRecursion(List **wqueue, Relation rel, ListCell *child; List *children; ! children = find_all_inheritors(relid, AccessExclusiveLock, NULL); /* * find_all_inheritors does the recursive search of the inheritance --- 3569,3575 ---- ListCell *child; List *children; ! children = find_all_inheritors(relid, lockmode, NULL); /* * find_all_inheritors does the recursive search of the inheritance *************** *** 3416,3422 **** ATSimpleRecursion(List **wqueue, Relation rel, /* find_all_inheritors already got lock */ childrel = relation_open(childrelid, NoLock); CheckTableNotInUse(childrel, "ALTER TABLE"); ! ATPrepCmd(wqueue, childrel, cmd, false, true); relation_close(childrel, NoLock); } } --- 3586,3592 ---- /* find_all_inheritors already got lock */ childrel = relation_open(childrelid, NoLock); CheckTableNotInUse(childrel, "ALTER TABLE"); ! ATPrepCmd(wqueue, childrel, cmd, false, true, lockmode); relation_close(childrel, NoLock); } } *************** *** 3432,3444 **** ATSimpleRecursion(List **wqueue, Relation rel, */ static void ATOneLevelRecursion(List **wqueue, Relation rel, ! AlterTableCmd *cmd) { Oid relid = RelationGetRelid(rel); ListCell *child; List *children; ! children = find_inheritance_children(relid, AccessExclusiveLock); foreach(child, children) { --- 3602,3614 ---- */ static void ATOneLevelRecursion(List **wqueue, Relation rel, ! AlterTableCmd *cmd, LOCKMODE lockmode) { Oid relid = RelationGetRelid(rel); ListCell *child; List *children; ! children = find_inheritance_children(relid, lockmode); foreach(child, children) { *************** *** 3448,3454 **** ATOneLevelRecursion(List **wqueue, Relation rel, /* find_inheritance_children already got lock */ childrel = relation_open(childrelid, NoLock); CheckTableNotInUse(childrel, "ALTER TABLE"); ! ATPrepCmd(wqueue, childrel, cmd, true, true); relation_close(childrel, NoLock); } } --- 3618,3624 ---- /* find_inheritance_children already got lock */ childrel = relation_open(childrelid, NoLock); CheckTableNotInUse(childrel, "ALTER TABLE"); ! ATPrepCmd(wqueue, childrel, cmd, true, true, lockmode); relation_close(childrel, NoLock); } } *************** *** 3584,3590 **** ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, colDefChild->inhcount = 1; colDefChild->is_local = false; ! ATOneLevelRecursion(wqueue, rel, childCmd); } else { --- 3754,3760 ---- colDefChild->inhcount = 1; colDefChild->is_local = false; ! ATOneLevelRecursion(wqueue, rel, childCmd, AccessExclusiveLock); } else { *************** *** 4392,4399 **** ATExecDropColumn(List **wqueue, Relation rel, const char *colName, * routines, we have to do this one level of recursion at a time; we can't * use find_all_inheritors to do it in one pass. */ ! children = find_inheritance_children(RelationGetRelid(rel), ! AccessExclusiveLock); if (children) { --- 4562,4568 ---- * routines, we have to do this one level of recursion at a time; we can't * use find_all_inheritors to do it in one pass. */ ! children = find_inheritance_children(RelationGetRelid(rel), AccessExclusiveLock); if (children) { *************** *** 4528,4534 **** ATExecDropColumn(List **wqueue, Relation rel, const char *colName, */ static void ATExecAddIndex(AlteredTableInfo *tab, Relation rel, ! IndexStmt *stmt, bool is_rebuild) { bool check_rights; bool skip_build; --- 4697,4703 ---- */ static void ATExecAddIndex(AlteredTableInfo *tab, Relation rel, ! IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode) { bool check_rights; bool skip_build; *************** *** 4571,4577 **** ATExecAddIndex(AlteredTableInfo *tab, Relation rel, */ static void ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, ! Constraint *newConstraint, bool recurse) { Assert(IsA(newConstraint, Constraint)); --- 4740,4746 ---- */ static void ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, ! Constraint *newConstraint, bool recurse, LOCKMODE lockmode) { Assert(IsA(newConstraint, Constraint)); *************** *** 4584,4590 **** ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, { case CONSTR_CHECK: ATAddCheckConstraint(wqueue, tab, rel, ! newConstraint, recurse, false); break; case CONSTR_FOREIGN: --- 4753,4759 ---- { case CONSTR_CHECK: ATAddCheckConstraint(wqueue, tab, rel, ! newConstraint, recurse, false, lockmode); break; case CONSTR_FOREIGN: *************** *** 4615,4621 **** ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, RelationGetNamespace(rel), NIL); ! ATAddForeignKeyConstraint(tab, rel, newConstraint); break; default: --- 4784,4790 ---- RelationGetNamespace(rel), NIL); ! ATAddForeignKeyConstraint(tab, rel, newConstraint, lockmode); break; default: *************** *** 4639,4645 **** ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, */ static void ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, ! Constraint *constr, bool recurse, bool recursing) { List *newcons; ListCell *lcon; --- 4808,4815 ---- */ static void ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, ! Constraint *constr, bool recurse, bool recursing, ! LOCKMODE lockmode) { List *newcons; ListCell *lcon; *************** *** 4694,4701 **** ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * routines, we have to do this one level of recursion at a time; we can't * use find_all_inheritors to do it in one pass. */ ! children = find_inheritance_children(RelationGetRelid(rel), ! AccessExclusiveLock); /* * If we are told not to recurse, there had better not be any child --- 4864,4870 ---- * routines, we have to do this one level of recursion at a time; we can't * use find_all_inheritors to do it in one pass. */ ! children = find_inheritance_children(RelationGetRelid(rel), lockmode); /* * If we are told not to recurse, there had better not be any child *************** *** 4721,4727 **** ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, /* Recurse to child */ ATAddCheckConstraint(wqueue, childtab, childrel, ! constr, recurse, true); heap_close(childrel, NoLock); } --- 4890,4896 ---- /* Recurse to child */ ATAddCheckConstraint(wqueue, childtab, childrel, ! constr, recurse, true, lockmode); heap_close(childrel, NoLock); } *************** *** 4736,4742 **** ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, */ static void ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, ! Constraint *fkconstraint) { Relation pkrel; int16 pkattnum[INDEX_MAX_KEYS]; --- 4905,4911 ---- */ static void ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, ! Constraint *fkconstraint, LOCKMODE lockmode) { Relation pkrel; int16 pkattnum[INDEX_MAX_KEYS]; *************** *** 4753,4766 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, Oid indexOid; Oid constrOid; ! /* ! * Grab an exclusive lock on the pk table, so that someone doesn't delete ! * rows out from under us. (Although a lesser lock would do for that ! * purpose, we'll need exclusive lock anyway to add triggers to the pk ! * table; trying to start with a lesser lock will just create a risk of ! * deadlock.) ! */ ! pkrel = heap_openrv(fkconstraint->pktable, AccessExclusiveLock); /* * Validity checks (permission checks wait till we have the column --- 4922,4928 ---- Oid indexOid; Oid constrOid; ! pkrel = heap_openrv(fkconstraint->pktable, lockmode); /* * Validity checks (permission checks wait till we have the column *************** *** 5324,5330 **** checkFkeyPermissions(Relation rel, int16 *attnums, int natts) * Scan the existing rows in a table to verify they meet a proposed FK * constraint. * ! * Caller must have opened and locked both relations. */ static void validateForeignKeyConstraint(Constraint *fkconstraint, --- 5486,5492 ---- * Scan the existing rows in a table to verify they meet a proposed FK * constraint. * ! * Caller must have opened and locked both relations appropriately. */ static void validateForeignKeyConstraint(Constraint *fkconstraint, *************** *** 5663,5670 **** ATExecDropConstraint(Relation rel, const char *constrName, * use find_all_inheritors to do it in one pass. */ if (is_check_constraint) ! children = find_inheritance_children(RelationGetRelid(rel), ! AccessExclusiveLock); else children = NIL; --- 5825,5831 ---- * use find_all_inheritors to do it in one pass. */ if (is_check_constraint) ! children = find_inheritance_children(RelationGetRelid(rel), AccessExclusiveLock); else children = NIL; *************** *** 5775,5781 **** static void ATPrepAlterColumnType(List **wqueue, AlteredTableInfo *tab, Relation rel, bool recurse, bool recursing, ! AlterTableCmd *cmd) { char *colName = cmd->name; TypeName *typeName = (TypeName *) cmd->def; --- 5936,5942 ---- ATPrepAlterColumnType(List **wqueue, AlteredTableInfo *tab, Relation rel, bool recurse, bool recursing, ! AlterTableCmd *cmd, LOCKMODE lockmode) { char *colName = cmd->name; TypeName *typeName = (TypeName *) cmd->def; *************** *** 5897,5903 **** ATPrepAlterColumnType(List **wqueue, * alter would put them out of step. */ if (recurse) ! ATSimpleRecursion(wqueue, rel, cmd, recurse); else if (!recursing && find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL) ereport(ERROR, --- 6058,6064 ---- * alter would put them out of step. */ if (recurse) ! ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode); else if (!recursing && find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL) ereport(ERROR, *************** *** 6304,6309 **** ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab) --- 6465,6474 ---- */ } + /* + * Called to add additional work items if ALTER TYPE touched indexed + * columns. Hence we always take AccessExclusiveLocks here. + */ static void ATPostAlterTypeParse(char *cmd, List **wqueue) { *** a/src/backend/commands/trigger.c --- b/src/backend/commands/trigger.c *************** *** 141,147 **** CreateTrigger(CreateTrigStmt *stmt, const char *queryString, ObjectAddress myself, referenced; ! rel = heap_openrv(stmt->relation, AccessExclusiveLock); if (rel->rd_rel->relkind != RELKIND_RELATION) ereport(ERROR, --- 141,154 ---- ObjectAddress myself, referenced; ! /* ! * ShareRowExclusiveLock is sufficient to prevent concurrent write activity ! * to the relation, and thus to lock out any operations that might want to ! * fire triggers on the relation. If we had ON SELECT triggers we would ! * need to take an AccessExclusiveLock to add one of those, just as we do ! * with ON SELECT rules. ! */ ! rel = heap_openrv(stmt->relation, ShareRowExclusiveLock); if (rel->rd_rel->relkind != RELKIND_RELATION) ereport(ERROR, *************** *** 417,423 **** CreateTrigger(CreateTrigStmt *stmt, const char *queryString, * can skip this for internally generated triggers, since the name * modification above should be sufficient. * ! * NOTE that this is cool only because we have AccessExclusiveLock on the * relation, so the trigger set won't be changing underneath us. */ if (!isInternal) --- 424,430 ---- * can skip this for internally generated triggers, since the name * modification above should be sufficient. * ! * NOTE that this is cool only because we have ShareRowExclusiveLock on the * relation, so the trigger set won't be changing underneath us. */ if (!isInternal) *************** *** 1051,1061 **** RemoveTriggerById(Oid trigOid) elog(ERROR, "could not find tuple for trigger %u", trigOid); /* ! * Open and exclusive-lock the relation the trigger belongs to. */ relid = ((Form_pg_trigger) GETSTRUCT(tup))->tgrelid; ! rel = heap_open(relid, AccessExclusiveLock); if (rel->rd_rel->relkind != RELKIND_RELATION) ereport(ERROR, --- 1058,1071 ---- elog(ERROR, "could not find tuple for trigger %u", trigOid); /* ! * Open and lock the relation the trigger belongs to. As in ! * CreateTrigger, this is sufficient to lock out all operations that ! * could fire or add triggers; but it would need to be revisited if ! * we had ON SELECT triggers. */ relid = ((Form_pg_trigger) GETSTRUCT(tup))->tgrelid; ! rel = heap_open(relid, ShareRowExclusiveLock); if (rel->rd_rel->relkind != RELKIND_RELATION) ereport(ERROR, *** a/src/backend/parser/parse_utilcmd.c --- b/src/backend/parser/parse_utilcmd.c *************** *** 53,58 **** --- 53,59 ---- #include "parser/parse_utilcmd.h" #include "parser/parser.h" #include "rewrite/rewriteManip.h" + #include "storage/lock.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/lsyscache.h" *************** *** 1528,1534 **** transformFKConstraints(ParseState *pstate, CreateStmtContext *cxt, } /* ! * transformIndexStmt - parse analysis for CREATE INDEX * * Note: this is a no-op for an index not using either index expressions or * a predicate expression. There are several code paths that create indexes --- 1529,1535 ---- } /* ! * transformIndexStmt - parse analysis for CREATE INDEX and ALTER TABLE * * Note: this is a no-op for an index not using either index expressions or * a predicate expression. There are several code paths that create indexes *************** *** 1554,1560 **** transformIndexStmt(IndexStmt *stmt, const char *queryString) * because addRangeTableEntry() would acquire only AccessShareLock, * leaving DefineIndex() needing to do a lock upgrade with consequent risk * of deadlock. Make sure this stays in sync with the type of lock ! * DefineIndex() wants. */ rel = heap_openrv(stmt->relation, (stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock)); --- 1555,1562 ---- * because addRangeTableEntry() would acquire only AccessShareLock, * leaving DefineIndex() needing to do a lock upgrade with consequent risk * of deadlock. Make sure this stays in sync with the type of lock ! * DefineIndex() wants. If we are being called by ALTER TABLE, we will ! * already hold a higher lock. */ rel = heap_openrv(stmt->relation, (stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock)); *************** *** 1919,1924 **** transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString) --- 1921,1927 ---- List *newcmds = NIL; bool skipValidation = true; AlterTableCmd *newcmd; + LOCKMODE lockmode; /* * We must not scribble on the passed-in AlterTableStmt, so copy it. (This *************** *** 1927,1939 **** transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString) stmt = (AlterTableStmt *) copyObject(stmt); /* ! * Acquire exclusive lock on the target relation, which will be held until * end of transaction. This ensures any decisions we make here based on * the state of the relation will still be good at execution. We must get ! * exclusive lock now because execution will; taking a lower grade lock ! * now and trying to upgrade later risks deadlock. */ ! rel = relation_openrv(stmt->relation, AccessExclusiveLock); /* Set up pstate */ pstate = make_parsestate(NULL); --- 1930,1948 ---- stmt = (AlterTableStmt *) copyObject(stmt); /* ! * Assign the appropriate lock level for this list of subcommands. ! */ ! lockmode = AlterTableGreatestLockLevel(stmt->cmds); ! ! /* ! * Acquire appropriate lock on the target relation, which will be held until * end of transaction. This ensures any decisions we make here based on * the state of the relation will still be good at execution. We must get ! * lock now because execution will later require it; taking a lower grade lock ! * now and trying to upgrade later risks deadlock. Any new commands we add ! * after this must not upgrade the lock level requested here. */ ! rel = relation_openrv(stmt->relation, lockmode); /* Set up pstate */ pstate = make_parsestate(NULL); *** a/src/backend/rewrite/rewriteDefine.c --- b/src/backend/rewrite/rewriteDefine.c *************** *** 236,246 **** DefineQueryRewrite(char *rulename, /* * If we are installing an ON SELECT rule, we had better grab * AccessExclusiveLock to ensure no SELECTs are currently running on the ! * event relation. For other types of rules, it might be sufficient to ! * grab ShareLock to lock out insert/update/delete actions. But for now, ! * let's just grab AccessExclusiveLock all the time. */ ! event_relation = heap_open(event_relid, AccessExclusiveLock); /* * Verify relation is of a type that rules can sensibly be applied to. --- 236,249 ---- /* * If we are installing an ON SELECT rule, we had better grab * AccessExclusiveLock to ensure no SELECTs are currently running on the ! * event relation. For other types of rules, it is sufficient to ! * grab ShareRowExclusiveLock to lock out insert/update/delete actions ! * and to ensure that we lock out current CREATE RULE statements. */ ! if (event_type == CMD_SELECT) ! event_relation = heap_open(event_relid, AccessExclusiveLock); ! else ! event_relation = heap_open(event_relid, ShareRowExclusiveLock); /* * Verify relation is of a type that rules can sensibly be applied to. *** a/src/backend/rewrite/rewriteSupport.c --- b/src/backend/rewrite/rewriteSupport.c *************** *** 70,81 **** SetRelationRuleStatus(Oid relationId, bool relHasRules, /* Do the update */ classForm->relhasrules = relHasRules; if (relIsBecomingView) classForm->relkind = RELKIND_VIEW; ! simple_heap_update(relationRelation, &tuple->t_self, tuple); ! /* Keep the catalog indexes up to date */ ! CatalogUpdateIndexes(relationRelation, tuple); } else { --- 70,85 ---- /* Do the update */ classForm->relhasrules = relHasRules; if (relIsBecomingView) + { classForm->relkind = RELKIND_VIEW; ! simple_heap_update(relationRelation, &tuple->t_self, tuple); ! /* Keep the catalog indexes up to date */ ! CatalogUpdateIndexes(relationRelation, tuple); ! } ! else ! heap_inplace_update(relationRelation, tuple); } else { *** a/src/backend/utils/adt/ri_triggers.c --- b/src/backend/utils/adt/ri_triggers.c *************** *** 2606,2612 **** RI_FKey_keyequal_upd_fk(Trigger *trigger, Relation fk_rel, * This is not a trigger procedure, but is called during ALTER TABLE * ADD FOREIGN KEY to validate the initial table contents. * ! * We expect that an exclusive lock has been taken on rel and pkrel; * hence, we do not need to lock individual rows for the check. * * If the check fails because the current user doesn't have permissions --- 2606,2612 ---- * This is not a trigger procedure, but is called during ALTER TABLE * ADD FOREIGN KEY to validate the initial table contents. * ! * We expect that a ShareRowExclusiveLock or higher has been taken on rel and pkrel; * hence, we do not need to lock individual rows for the check. * * If the check fails because the current user doesn't have permissions *** a/src/include/commands/tablecmds.h --- b/src/include/commands/tablecmds.h *************** *** 15,20 **** --- 15,21 ---- #define TABLECMDS_H #include "nodes/parsenodes.h" + #include "storage/lock.h" #include "utils/relcache.h" *************** *** 24,29 **** extern void RemoveRelations(DropStmt *drop); --- 25,32 ---- extern void AlterTable(AlterTableStmt *stmt); + extern LOCKMODE AlterTableGreatestLockLevel(List *cmds); + extern void ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing); extern void AlterTableInternal(Oid relid, List *cmds, bool recurse);
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers