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