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