ALTER TABLE SET STATISTICS requires AccessExclusiveLock
in [PgSql]
|
Prev: pg_dump and join aliases (was Re: [BUGS] ERROR: cannot handle unplanned sub-select)
Next: patch: preload dictionary new version
From: Andres Freund on 16 Jul 2010 14:41 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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Andres Freund on 16 Jul 2010 15:10 On Friday 16 July 2010 20:41:44 Andres Freund wrote: > >> ! */ > >> ! 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 */ Another remark: Imho it would be usefull to keep that list in same order as in the enum - currently its hard to make sure no case is missing. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Andres Freund on 16 Jul 2010 15:38 On Friday 16 July 2010 21:12:33 Simon Riggs wrote: > On Fri, 2010-07-16 at 20:41 +0200, Andres Freund wrote: > > 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... > > This is only for adding a constraint, not removing it. Join removal > would be possible after the ALTER finishes, but won't change plans > already in progress. The idea is to minimise the impact, not maximise > the benefit of the newly added constraint; I don't think we should block > all queries just because a few might benefit. Its not about benefit, its about correctness: CREATE TABLE testsnap(t int); INSERT INTO testsnap VALUES(1),(1); T1: test=# BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; BEGIN Time: 0.853 ms test=# explain analyze SELECT t1.* FROM testsnap t1 LEFT JOIN testsnap t2 USING(t); QUERY PLAN --------------------------------------------------------------------------------------------------------------------- Merge Left Join (cost=337.49..781.49 rows=28800 width=4) (actual time=0.090..0.118 rows=4 loops=1) Merge Cond: (t1.t = t2.t) -> Sort (cost=168.75..174.75 rows=2400 width=4) (actual time=0.049..0.051 rows=2 loops=1) Sort Key: t1.t Sort Method: quicksort Memory: 25kB -> Seq Scan on testsnap t1 (cost=0.00..34.00 rows=2400 width=4) (actual time=0.018..0.023 rows=2 loops=1) -> Sort (cost=168.75..174.75 rows=2400 width=4) (actual time=0.026..0.033 rows=3 loops=1) Sort Key: t2.t Sort Method: quicksort Memory: 25kB -> Seq Scan on testsnap t2 (cost=0.00..34.00 rows=2400 width=4) (actual time=0.005..0.009 rows=2 loops=1) Total runtime: 0.279 ms (11 rows) T2: test=# DELETE FROM testsnap; DELETE 2 Time: 1.184 ms test=# ALTER TABLE testsnap ADD CONSTRAINT t unique(t); NOTICE: 00000: ALTER TABLE / ADD UNIQUE will create implicit index "t" for table "testsnap" LOCATION: DefineIndex, indexcmds.c:471 ALTER TABLE Time: 45.639 ms T1: Time: 1.948 ms test=# explain analyze SELECT t1.* FROM testsnap t1 LEFT JOIN testsnap t2 USING(t); QUERY PLAN ----------------------------------------------------------------------------------------------------- Seq Scan on testsnap t1 (cost=0.00..1.02 rows=2 width=4) (actual time=0.013..0.016 rows=2 loops=1) Total runtime: 0.081 ms (2 rows) Time: 2.004 ms test=# boom. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Andres Freund on 16 Jul 2010 15:43 On Friday 16 July 2010 21:15:44 Simon Riggs wrote: > On Fri, 2010-07-16 at 21:10 +0200, Andres Freund wrote: > > On Friday 16 July 2010 20:41:44 Andres Freund wrote: > > > >> ! */ > > > >> ! 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 */ > > > > Another remark: > > > > Imho it would be usefull to keep that list in same order as in the enum - > > currently its hard to make sure no case is missing. > > Not really; the default case is to reject, so any full test suite will > pick that up. > > The cases are ordered by resulting lock type, which seemed the best way > to check we didn't accidentally assign an incorrect lock type. Well, I meant ordering it correctly inside the locktypes, sorry for the inprecision. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Andres Freund on 16 Jul 2010 17:03
On Friday 16 July 2010 22:24:32 Simon Riggs wrote: > On Fri, 2010-07-16 at 21:38 +0200, Andres Freund wrote: > > boom > > Your test case would still occur in the case where the first query had > not been executed against the same table. So the test case illustrates a > failing of join removal, not of this patch. Well, yes. Thats a well known (and documented) issue of pg's serialized transactions - which you can protect against quite easily (see the trunctate docs for example). The difference is that I know of no sensible way you sensibly could protect against such issues with the patch applied while its easy before(LOCK TABLE .... IN SHARE MODE for all used tables). I know of several sites which have *long* running serialized transactions open for analysis and I know there have been other cases of it. Sure its not that bad, but at least it needs to get documented imho. Likely others should chime in here ;-) What could the join removal path (and similar places) *possibly* do against such a case? Without stopping to use SnapshotNow I dont see any way :-( Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |