Hi Simon,
Your patch implements part of a feature I desire greatly - thanks!
Some comments:
On Thursday 15 July 2010 11:24:27 Simon Riggs wrote:
>> ! LOCKMODE
>> ! AlterTableGreatestLockLevel(List *cmds)
>> ! {
>> ! ListCell *lcmd;
>> ! LOCKMODE lockmode = ShareUpdateExclusiveLock; /* default for compiler
>> */
Actually its not only for the compiler, its necessary for correctness
as you omit the default at least in the AT_AddConstraint case.
>> !
>> ! 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;
You argue above that you cant change SET [NOT] NULL to be less
restrictive because it might change plans - isnt that true for some of the
above cases as well?
For example UNIQUE/PRIMARY might make join removal possible - which could
only be valid after "invalid" tuples where deleted earlier in that
transaction. Another case which it influences are grouping plans...
So I think the only case where its actually possible to lower the
level is CONSTR_EXCLUSION and _FOREIGN.
The latter might get impossible soon by planner improvements (Peter's
functional dependencies patch for example).
Sorry, thats it for now...
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
