Reduce lock levels of some trigger DDL and add FKs

Reduce lock levels to ShareRowExclusive for the following SQL
 CREATE TRIGGER (but not DROP or ALTER)
 ALTER TABLE ENABLE TRIGGER
 ALTER TABLE DISABLE TRIGGER
 ALTER TABLE … ADD CONSTRAINT FOREIGN KEY

Original work by Simon Riggs, extracted and refreshed by Andreas Karlsson
New test cases added by Andreas Karlsson
Reviewed by Noah Misch, Andres Freund, Michael Paquier and Simon Riggs

doc/src/sgml/mvcc.sgml

@@ -909,9 +909,9 @@ ERROR:  could not serialize access due to read/write dependencies among transact
         </para>
 
         <para>
-         This lock mode is not automatically acquired by any
+         Acquired by <command>CREATE TRIGGER</command> and many forms of
-         <productname>PostgreSQL</productname> command.
+         <command>ALTER TABLE</command> (see <xref linkend="SQL-ALTERTABLE">).
-        </para>
+        </para>>
        </listitem>
       </varlistentry>
 
@@ -958,9 +958,9 @@ ERROR:  could not serialize access due to read/write dependencies among transact
          <command>TRUNCATE</command>, <command>REINDEX</command>,
          <command>CLUSTER</command>, and <command>VACUUM FULL</command>
          commands. Many forms of <command>ALTER TABLE</> also acquire
-         a lock at this level (see <xref linkend="SQL-ALTERTABLE">).
+         a lock at this level. This is also the default lock mode for
-         This is also the default lock mode for <command>LOCK TABLE</command>
+         <command>LOCK TABLE</command> statements that do not specify
-         statements that do not specify a mode explicitly.
+         a mode explicitly.
         </para>
        </listitem>
       </varlistentry>

doc/src/sgml/ref/alter_table.sgml

@@ -406,6 +406,9 @@ ALTER TABLE ALL IN TABLESPACE <replaceable class="PARAMETER">name</replaceable>
       mode, and triggers configured as <literal>ENABLE ALWAYS</literal> will
       fire regardless of the current replication mode.
      </para>
+     <para>
+      This command acquires a <literal>SHARE ROW EXCLUSIVE</literal> lock.
+     </para>
     </listitem>
    </varlistentry>
 

src/backend/commands/tablecmds.c

@@ -2892,13 +2892,8 @@ AlterTableGetLockLevel(List *cmds)
 				break;
 
 				/*
-				 * These subcommands affect write operations only. XXX
+				 * These subcommands affect write operations only.
-				 * 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_EnableTrig:
 			case AT_EnableAlwaysTrig:
 			case AT_EnableReplicaTrig:
@@ -2907,6 +2902,14 @@ 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_AlterConstraint:
 			case AT_AddIndex:	/* from ADD CONSTRAINT */
 			case AT_AddIndexConstraint:
@@ -2918,6 +2921,9 @@ AlterTableGetLockLevel(List *cmds)
 				break;
 
 			case AT_AddConstraint:
+			case AT_ProcessedConstraint:		/* becomes AT_AddConstraint */
+			case AT_AddConstraintRecurse:		/* becomes AT_AddConstraint */
+			case AT_ReAddConstraint:	/* becomes AT_AddConstraint */
 				if (IsA(cmd->def, Constraint))
 				{
 					Constraint *con = (Constraint *) cmd->def;
@@ -2943,11 +2949,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
+							 * as strong as CREATE TRIGGER.
-							 * down to ShareRowExclusiveLock though trigger
-							 * info is accessed by pg_get_triggerdef
 							 */
-							cmd_lockmode = AccessExclusiveLock;
+							cmd_lockmode = ShareRowExclusiveLock;
 							break;
 
 						default:
@@ -6193,16 +6197,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
+	 * Grab ShareRowExclusiveLock on the pk table, so that someone doesn't
-	 * rows out from under us. (Although a lesser lock would do for that
+	 * delete rows out from under us.
-	 * 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);
+		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

src/backend/commands/trigger.c

@@ -165,9 +165,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

src/test/isolation/isolation_schedule

@@ -34,4 +34,7 @@ test: skip-locked-3
 test: skip-locked-4
 test: drop-index-concurrently-1
 test: alter-table-1
+test: alter-table-2
+test: alter-table-3
+test: create-trigger
 test: timeouts

src/test/regress/expected/alter_table.out

@@ -1958,8 +1958,8 @@ create trigger ttdummy
 	ttdummy (1, 1);
 select * from my_locks order by 1;
   relname  |     max_lockmode      
------------+---------------------
+-----------+-----------------------
- alterlock | AccessExclusiveLock
+ alterlock | ShareRowExclusiveLock
 (1 row)
 
 rollback;
@@ -1972,9 +1972,9 @@ 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
+ alterlock       | ShareRowExclusiveLock
- alterlock2      | AccessExclusiveLock
+ alterlock2      | ShareRowExclusiveLock
  alterlock2_pkey | AccessShareLock
  alterlock_pkey  | AccessShareLock
 (4 rows)
@@ -1985,9 +1985,9 @@ 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
+ alterlock  | ShareRowExclusiveLock
- alterlock2 | AccessExclusiveLock
+ alterlock2 | ShareRowExclusiveLock
 (2 rows)
 
 commit;