Hi,
I have been thinking about why we need to grab an AccessExclusiveLock on
the table with the PK when we add a foreign key. Adding new tables with
foreign keys to old ones is common so it would be really nice if the
lock strength could be reduced.
A comment in the code claims that we need to lock so no rows are deleted
under us and that adding a trigger will lock in AccessExclusive anyway.
But with MVCC catalog access and the removal of SnapshotNow I do not see
why adding a trigger would require an exclusive lock. Just locking for
data changes should be enough.
Looking at the code the only see the duplicate name check use the fact
that we have grabbed an exclusive lock and that should work anyway due
to the unique constraint, but since I am pretty new to the code base I
could be missing something obvious. I have attached a proof of concept
patch which reduces the lock strength to ShareLock.
What do you say? Am I missing something?
My gut feel also says that if we know it is a RI trigger we are adding
then AccessShare should be enough for the PK table, since we could rely
on row locks to prevent rows from being deleted. It would be really nice
though if this was possible since this would make it possible to add a
new table with foreign keys and data without locking anything more than
the referred rows.
Andreas
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index ecdff1e..49ad323
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** ATAddForeignKeyConstraint(AlteredTableIn
*** 6016,6031 ****
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.)
*/
if (OidIsValid(fkconstraint->old_pktable_oid))
! pkrel = heap_open(fkconstraint->old_pktable_oid, AccessExclusiveLock);
else
! pkrel = heap_openrv(fkconstraint->pktable, AccessExclusiveLock);
/*
* Validity checks (permission checks wait till we have the column
--- 6016,6028 ----
ListCell *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop);
/*
! * 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, ShareLock);
else
! pkrel = heap_openrv(fkconstraint->pktable, ShareLock);
/*
* Validity checks (permission checks wait till we have the column
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
new file mode 100644
index f4c0ffa..214755d
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
*************** CreateTrigger(CreateTrigStmt *stmt, cons
*** 157,165 ****
referenced;
if (OidIsValid(relOid))
! rel = heap_open(relOid, AccessExclusiveLock);
else
! rel = heap_openrv(stmt->relation, AccessExclusiveLock);
/*
* Triggers must be on tables or views, and there are additional
--- 157,165 ----
referenced;
if (OidIsValid(relOid))
! rel = heap_open(relOid, ShareLock);
else
! rel = heap_openrv(stmt->relation, ShareLock);
/*
* Triggers must be on tables or views, and there are additional
*************** CreateTrigger(CreateTrigStmt *stmt, cons
*** 525,532 ****
* 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.
*/
if (!isInternal)
{
--- 525,531 ----
* can skip this for internally generated triggers, since the name
* modification above should be sufficient.
*
! * NOTE that this is cool only because of the unique contraint.
*/
if (!isInternal)
{
*************** RemoveTriggerById(Oid trigOid)
*** 1102,1108 ****
*/
relid = ((Form_pg_trigger) GETSTRUCT(tup))->tgrelid;
! rel = heap_open(relid, AccessExclusiveLock);
if (rel->rd_rel->relkind != RELKIND_RELATION &&
rel->rd_rel->relkind != RELKIND_VIEW &&
--- 1101,1107 ----
*/
relid = ((Form_pg_trigger) GETSTRUCT(tup))->tgrelid;
! rel = heap_open(relid, ShareLock);
if (rel->rd_rel->relkind != RELKIND_RELATION &&
rel->rd_rel->relkind != RELKIND_VIEW &&
*************** renametrig(RenameStmt *stmt)
*** 1257,1263 ****
* Look up name, check permissions, and acquire lock (which we will NOT
* release until end of transaction).
*/
! relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
false, false,
RangeVarCallbackForRenameTrigger,
NULL);
--- 1256,1262 ----
* Look up name, check permissions, and acquire lock (which we will NOT
* release until end of transaction).
*/
! relid = RangeVarGetRelidExtended(stmt->relation, ShareLock,
false, false,
RangeVarCallbackForRenameTrigger,
NULL);
*************** renametrig(RenameStmt *stmt)
*** 1271,1278 ****
* 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.
*/
tgrel = heap_open(TriggerRelationId, RowExclusiveLock);
--- 1270,1276 ----
* on tgrelid/tgname would complain anyway) and to ensure a trigger does
* exist with oldname.
*
! * NOTE that this is cool only because there is a unique constraint.
*/
tgrel = heap_open(TriggerRelationId, RowExclusiveLock);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
new file mode 100644
index 10f45f2..636b951
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
*************** create trigger ttdummy
*** 1935,1943 ****
execute procedure
ttdummy (1, 1);
select * from my_locks order by 1;
! relname | max_lockmode
! -----------+---------------------
! alterlock | AccessExclusiveLock
(1 row)
rollback;
--- 1935,1943 ----
execute procedure
ttdummy (1, 1);
select * from my_locks order by 1;
! relname | max_lockmode
! -----------+--------------
! alterlock | ShareLock
(1 row)
rollback;
*************** alter table alterlock2 add foreign key (
*** 1951,1957 ****
select * from my_locks order by 1;
relname | max_lockmode
-----------------+---------------------
! alterlock | AccessExclusiveLock
alterlock2 | AccessExclusiveLock
alterlock2_pkey | AccessShareLock
alterlock_pkey | AccessShareLock
--- 1951,1957 ----
select * from my_locks order by 1;
relname | max_lockmode
-----------------+---------------------
! alterlock | ShareLock
alterlock2 | AccessExclusiveLock
alterlock2_pkey | AccessShareLock
alterlock_pkey | AccessShareLock
*************** add constraint alterlock2nv foreign key
*** 1964,1970 ****
select * from my_locks order by 1;
relname | max_lockmode
------------+---------------------
! alterlock | AccessExclusiveLock
alterlock2 | AccessExclusiveLock
(2 rows)
--- 1964,1970 ----
select * from my_locks order by 1;
relname | max_lockmode
------------+---------------------
! alterlock | ShareLock
alterlock2 | AccessExclusiveLock
(2 rows)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
