Reducing some DDL Locks to ShareLock - Mailing list pgsql-hackers

It seems possible to change some DDL commands/subcommands to use a
ShareLock rather than an AccessExclusiveLock. Enclosed patch implements
this reduction for CREATE TRIGGER, CREATE RULE and ALTER TABLE.

We can relax locking as long as we can verify that the changes effect
DML statements *only*. I've marked the commands/subcommands this can be
applied to with an "s" (for ShareLock) and noted others that must remain
"x" (for AccessExclusiveLock).

s    CREATE RULE (only non-ON SELECT rules)

s    CREATE TRIGGER

ALTER TABLE
x    RENAME [ COLUMN ] column TO new_column
x    RENAME TO new_name
x    SET SCHEMA new_schema
x    ADD [ COLUMN ] column type [ column_constraint [ ... ] ]
x    DROP [ COLUMN ] column [ RESTRICT | CASCADE ]
x    ALTER [ COLUMN ] column TYPE type [ USING expression ]
x    ALTER [ COLUMN ] column SET DEFAULT expression
x    ALTER [ COLUMN ] column DROP DEFAULT
s    ALTER [ COLUMN ] column SET NOT NULL
x    ALTER [ COLUMN ] column DROP NOT NULL
s    ALTER [ COLUMN ] column SET STATISTICS integer
s    ALTER [ COLUMN ] column SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
s    ADD table_constraint
x    DROP CONSTRAINT constraint_name [ RESTRICT | CASCADE ]
s    DISABLE TRIGGER [ trigger_name | ALL | USER ]
s    ENABLE TRIGGER [ trigger_name | ALL | USER ]
s    ENABLE REPLICA TRIGGER trigger_name
s    ENABLE ALWAYS TRIGGER trigger_name
x    DISABLE RULE rewrite_rule_name
x    ENABLE RULE rewrite_rule_name
x    ENABLE REPLICA RULE rewrite_rule_name
x    ENABLE ALWAYS RULE rewrite_rule_name
s    CLUSTER ON index_name
s    SET WITHOUT CLUSTER
x    SET WITHOUT OIDS
s    SET ( storage_parameter = value [, ... ] )
s    RESET ( storage_parameter [, ... ] )
x    INHERIT parent_table
x    NO INHERIT parent_table
x    OWNER TO new_owner
x    SET TABLESPACE new_tablespace

If an ALTER TABLE has more than one sub-command we take the highest lock
to cover the execution of all sub-commands, since we want to avoid lock
upgrades and deadlocks.

e.g.
ALTER TABLE foo ADD PRIMARY KEY (col1); will take ShareLock
whereas
ALTER TABLE foo ADD PRIMARY KEY (col1), INHERIT bar; will require an
AccessExclusiveLock

I've checked definitions of these subcommands here
http://developer.postgresql.org/pgdocs/postgres/sql-altertable.html
As noted, Rules can be ON SELECT, so we can't enable/disable them
without an AccessExclusiveLock. Other than that, many of the subcommands
don't change the actual table data, they just change settings for future
DML changes to the table.

This will
* allow ALTER TABLE ADD FOREIGN KEY to run in parallel with pg_restore
* allow trigger-based replication systems to add triggers more easily
* reduce the number of AccessExclusiveLocks that have to be dealt with
in Hot Standby mode.

Couple of points of note:

* Patch implements this by changing pg_class.reltriggers so it only has
two values, 0 or 1. Do we want to change this to a boolean, or leave as
an int2 for backwards compatibility? Code works either way. Enclosed
patch keeps it as an int2 for now, easily changed on request.

* transformIndexStatement() and DefineIndex() both take the wrong level
of locks in existing code, when called from ALTER TABLE. Not sure
whether this should be fixed in conjunction with these changes or not.
Doesn't seem to make any difference, since the "wrong level" is taken
after the initial lock is taken, so we never actually notice.

* ATRewriteTables() uses the wrong lock level in existing code when we
call validateForeignKeyConstraint(). It uses RowShareLock(), which is
not sufficient to stop DML when checking using the SELECT in
RI_Initial_Check(). Again, at present we take an AccessExclusiveLock
first, so not a problem now. Changed to ShareLock in patch.

Patch passes make check on an assert build, plus manual observation of
lock order and locks held. I've also introduced two new lock functions
LockHeldByMe() and RelationLockedByMe() to allow Assert() checking of
lock levels held. These are added in key points in ALTER TABLE code to
ensure no mistakes are made about required lock levels.

 doc/src/sgml/mvcc.sgml              |   17 !!
 src/backend/commands/tablecmds.c    |  278 ++++++!!!!!!!!!!!!!!!!!!!!!
 src/backend/commands/trigger.c      |   88 -!!!!!!!!!!
 src/backend/parser/parse_utilcmd.c  |   21 !!
 src/backend/rewrite/rewriteDefine.c |   10 !
 src/backend/storage/lmgr/lmgr.c     |   12 +
 src/backend/storage/lmgr/lock.c     |   30 +++
 src/backend/utils/adt/ri_triggers.c |    2
 src/include/commands/tablecmds.h    |    2
 src/include/storage/lmgr.h          |    2
 src/include/storage/lock.h          |    1
 11 files changed, 107 insertions(+), 11 deletions(-), 345 mods(!)

Patch is overall fairly straightforward:
* Assess required lock level
* Pass down decision through APIs to allow recursing to inherited tables
* Change comments at various places (or not)
* Trigger code needs to work around no knowing how many triggers are
present, so relcache now handled similarly to Rules

Patch correctly handles lock levels in cases such a reindex of indexes
following ALTER TYPE of an indexed column. It might appear from the code
that this had been missed, but tracing code shows it is correctly set.

There is still scope for an "ALTER TABLE CONCURRENTLY" command as has
been suggested. This proposal neither blocks that nor is blocked by it.
Not every application can be changed to accept new syntax, nor is it
desirable for every application to issue these commands only in
top-level transactions.

There are probably even more tweaks that we could do than the above
list, but the above seems fairly straightforward for now. We can always
do more later.

--
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Training, Services and Support