Re: ALTER TABLE SET STATISTICS requires AccessExclusiveLock

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