Hi,
I have attached a patch with the current status of my work on reducing
the lock level of trigger and foreign key related DDL.
This commit reduces the lock level of the following commands from ACCESS
EXCLUSIVE to SHARE ROW EXCLUSIVE, plus that it does the same for the
referred table of the foreign key.
ADD TRIGGER
ALTER TRIGGER
ALTER TABLE ... ADD FOREIGN KEY
ALTER TABLE ... DISABLE TRIGGER
ALTER TABLE ... ENABLE TRIGGER
The patch does currently not reducing the lock level of the following
two commands, but I think it would be really nice to fix those too and
here I would like some feedback and ideas.
DROP TRIGGER
ALTER TABLE ... DROP CONSTRAINT -- For foreign keys
Foreign keys and triggers are fixed at the same time because foreign
keys are implemented with two triggers, one at each of the involved tables.
The idea behind the patch is that since we started using MVCC snapshots
we no longer need to hold an exclusive lock on the table when adding a
trigger. Theoretically we only need to lock out writers of the rows of
the table (SHARE ROW EXCLUSIVE/SHARE), but as noted by Robert and Noah
just reducing the lock level will break pg_dump since
pg_get_triggerdef() does not use the current snapshot when reading the
catalog.
I have fixed pg_get_triggerdef() to use the snapshot like how
e5550d5fec66aa74caad1f79b79826ec64898688 fixed pg_get_constraintdef(),
and this fixes the code for the ALTER TRIGGER case (ADD TRIGGER was
already safe). But to fix it for the DROP TRIGGER case we need to also
make the dumping of the WHEN clause (which is dumped by get_rule_expr())
use the current snapshot.
get_rule_expr() relies heavily on the catcache which to me does not look
like it could easily be (and probably not even should be) made to use
the current snapshot. Refactoring ruleutils.c to rely less no the
catcache seems like a reasonable thing to do if we want to reduce
weirdness of how it ignores MVCC but it is quite a bit of work and I
fear it could give us performance regressions.
Do you have any ideas for how to fix get_rule_expr()? Is this patch
worthwhile even without reducing the lock levels of the drop commands?
Andreas
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1e737a0..ece6ed5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2862,13 +2862,8 @@ AlterTableGetLockLevel(List *cmds)
break;
/*
- * These subcommands affect write operations only. XXX
- * Theoretically, these could be ShareRowExclusiveLock.
+ * These subcommands affect write operations only.
*/
- case AT_ColumnDefault:
- case AT_ProcessedConstraint: /* becomes AT_AddConstraint */
- case AT_AddConstraintRecurse: /* becomes AT_AddConstraint */
- case AT_ReAddConstraint: /* becomes AT_AddConstraint */
case AT_EnableTrig:
case AT_EnableAlwaysTrig:
case AT_EnableReplicaTrig:
@@ -2877,6 +2872,17 @@ AlterTableGetLockLevel(List *cmds)
case AT_DisableTrig:
case AT_DisableTrigAll:
case AT_DisableTrigUser:
+ cmd_lockmode = ShareRowExclusiveLock;
+ break;
+
+ /*
+ * These subcommands affect write operations only. XXX
+ * Theoretically, these could be ShareRowExclusiveLock.
+ */
+ case AT_ColumnDefault:
+ case AT_ProcessedConstraint: /* becomes AT_AddConstraint */
+ case AT_AddConstraintRecurse: /* becomes AT_AddConstraint */
+ case AT_ReAddConstraint: /* becomes AT_AddConstraint */
case AT_AlterConstraint:
case AT_AddIndex: /* from ADD CONSTRAINT */
case AT_AddIndexConstraint:
@@ -2913,11 +2919,9 @@ AlterTableGetLockLevel(List *cmds)
/*
* 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. XXX Might be set
- * down to ShareRowExclusiveLock though trigger
- * info is accessed by pg_get_triggerdef
+ * as strong as CREATE TRIGGER.
*/
- cmd_lockmode = AccessExclusiveLock;
+ cmd_lockmode = ShareRowExclusiveLock;
break;
default:
@@ -6034,16 +6038,13 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
ListCell *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop);
/*
- * Grab an exclusive lock on the pk table, so that someone doesn't delete
- * rows out from under us. (Although a lesser lock would do for that
- * purpose, we'll need exclusive lock anyway to add triggers to the pk
- * table; trying to start with a lesser lock will just create a risk of
- * deadlock.)
+ * Grab an share lock on the pk table, so that someone doesn't delete
+ * rows out from under us.
*/
if (OidIsValid(fkconstraint->old_pktable_oid))
- pkrel = heap_open(fkconstraint->old_pktable_oid, AccessExclusiveLock);
+ pkrel = heap_open(fkconstraint->old_pktable_oid, ShareRowExclusiveLock);
else
- pkrel = heap_openrv(fkconstraint->pktable, AccessExclusiveLock);
+ pkrel = heap_openrv(fkconstraint->pktable, ShareRowExclusiveLock);
/*
* Validity checks (permission checks wait till we have the column
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index ebccfea..b2d5a52 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -158,9 +158,9 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
referenced;
if (OidIsValid(relOid))
- rel = heap_open(relOid, AccessExclusiveLock);
+ rel = heap_open(relOid, ShareRowExclusiveLock);
else
- rel = heap_openrv(stmt->relation, AccessExclusiveLock);
+ rel = heap_openrv(stmt->relation, ShareRowExclusiveLock);
/*
* Triggers must be on tables or views, and there are additional
@@ -526,8 +526,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
* can skip this for internally generated triggers, since the name
* modification above should be sufficient.
*
- * NOTE that this is cool only because we have AccessExclusiveLock on the
- * relation, so the trigger set won't be changing underneath us.
+ * NOTE that this is cool only because of the unique contraint.
*/
if (!isInternal)
{
@@ -1258,7 +1257,7 @@ renametrig(RenameStmt *stmt)
* Look up name, check permissions, and acquire lock (which we will NOT
* release until end of transaction).
*/
- relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
+ relid = RangeVarGetRelidExtended(stmt->relation, ShareRowExclusiveLock,
false, false,
RangeVarCallbackForRenameTrigger,
NULL);
@@ -1272,8 +1271,7 @@ renametrig(RenameStmt *stmt)
* on tgrelid/tgname would complain anyway) and to ensure a trigger does
* exist with oldname.
*
- * NOTE that this is cool only because we have AccessExclusiveLock on the
- * relation, so the trigger set won't be changing underneath us.
+ * NOTE that this is cool only because there is a unique constraint.
*/
tgrel = heap_open(TriggerRelationId, RowExclusiveLock);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 24ade6c..4deb9c2 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -699,7 +699,8 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty)
HeapTuple ht_trig;
Form_pg_trigger trigrec;
StringInfoData buf;
- Relation tgrel;
+ Snapshot snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ Relation tgrel = heap_open(TriggerRelationId, AccessShareLock);
ScanKeyData skey[1];
SysScanDesc tgscan;
int findx = 0;
@@ -710,18 +711,18 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty)
/*
* Fetch the pg_trigger tuple by the Oid of the trigger
*/
- tgrel = heap_open(TriggerRelationId, AccessShareLock);
-
ScanKeyInit(&skey[0],
ObjectIdAttributeNumber,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(trigid));
tgscan = systable_beginscan(tgrel, TriggerOidIndexId, true,
- NULL, 1, skey);
+ snapshot, 1, skey);
ht_trig = systable_getnext(tgscan);
+ UnregisterSnapshot(snapshot);
+
if (!HeapTupleIsValid(ht_trig))
elog(ERROR, "could not find tuple for trigger %u", trigid);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index d233710..1f7c265 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1952,9 +1952,9 @@ create trigger ttdummy
execute procedure
ttdummy (1, 1);
select * from my_locks order by 1;
- relname | max_lockmode
------------+---------------------
- alterlock | AccessExclusiveLock
+ relname | max_lockmode
+-----------+-----------------------
+ alterlock | ShareRowExclusiveLock
(1 row)
rollback;
@@ -1966,10 +1966,10 @@ select * from my_locks order by 1;
alter table alterlock2 add foreign key (f1) references alterlock (f1);
select * from my_locks order by 1;
- relname | max_lockmode
------------------+---------------------
- alterlock | AccessExclusiveLock
- alterlock2 | AccessExclusiveLock
+ relname | max_lockmode
+-----------------+-----------------------
+ alterlock | ShareRowExclusiveLock
+ alterlock2 | ShareRowExclusiveLock
alterlock2_pkey | AccessShareLock
alterlock_pkey | AccessShareLock
(4 rows)
@@ -1979,10 +1979,10 @@ begin;
alter table alterlock2
add constraint alterlock2nv foreign key (f1) references alterlock (f1) NOT VALID;
select * from my_locks order by 1;
- relname | max_lockmode
-------------+---------------------
- alterlock | AccessExclusiveLock
- alterlock2 | AccessExclusiveLock
+ relname | max_lockmode
+------------+-----------------------
+ alterlock | ShareRowExclusiveLock
+ alterlock2 | ShareRowExclusiveLock
(2 rows)
commit;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
