Reduce lock levels of some ALTER TABLE cmds

VALIDATE CONSTRAINT

CLUSTER ON
SET WITHOUT CLUSTER

ALTER COLUMN SET STATISTICS
ALTER COLUMN SET ()
ALTER COLUMN RESET ()

All other sub-commands use AccessExclusiveLock

Simon Riggs and Noah Misch

Reviews by Robert Haas and Andres Freund

doc/src/sgml/mvcc.sgml

@@ -865,7 +865,9 @@ ERROR:  could not serialize access due to read/write dependencies among transact
         <para>
          Acquired by <command>VACUUM</command> (without <option>FULL</option>),
          <command>ANALYZE</>, <command>CREATE INDEX CONCURRENTLY</>, and
-         some forms of <command>ALTER TABLE</command>.
+         <command>ALTER TABLE VALIDATE</command> and other
+         <command>ALTER TABLE</command> variants (for full details see
+         <xref linkend="SQL-ALTERTABLE">).
         </para>
        </listitem>
       </varlistentry>
@@ -951,10 +953,11 @@ ERROR:  could not serialize access due to read/write dependencies among transact
         </para>
 
         <para>
-         Acquired by the <command>ALTER TABLE</>, <command>DROP TABLE</>,
+         Acquired by the <command>DROP TABLE</>,
          <command>TRUNCATE</command>, <command>REINDEX</command>,
          <command>CLUSTER</command>, and <command>VACUUM FULL</command>
-         commands.
+         commands. Many forms of <command>ALTER TABLE</> also acquire
+         a lock at this level (see <xref linkend="SQL-ALTERTABLE">).
          This is also the default lock mode for <command>LOCK TABLE</command>
          statements that do not specify a mode explicitly.
         </para>

doc/src/sgml/ref/alter_table.sgml

@@ -84,7 +84,10 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
 
   <para>
    <command>ALTER TABLE</command> changes the definition of an existing table.
-   There are several subforms:
+   There are several subforms described below. Note that the lock level required
+   may differ for each subform. An <literal>ACCESS EXCLUSIVE</literal> lock is held
+   unless explicitly noted. When multiple subcommands are listed, the lock
+   held will be the strictest one required from any subcommand.
 
   <variablelist>
    <varlistentry>
@@ -181,6 +184,9 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
       <productname>PostgreSQL</productname> query planner, refer to
       <xref linkend="planner-stats">.
      </para>
+     <para>
+      SET STATISTICS acquires a <literal>SHARE UPDATE EXCLUSIVE</literal> lock.
+     </para>
     </listitem>
    </varlistentry>
 
@@ -213,6 +219,10 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
       of statistics by the <productname>PostgreSQL</productname> query
       planner, refer to <xref linkend="planner-stats">.
      </para>
+     <para>
+      Changing per-attribute options acquires a
+      <literal>SHARE UPDATE EXCLUSIVE</literal> lock.
+     </para>
     </listitem>
    </varlistentry>
 
@@ -338,11 +348,17 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
       Nothing happens if the constraint is already marked valid.
      </para>
      <para>
-      Validation can be a long process on larger tables and currently requires
-      an <literal>ACCESS EXCLUSIVE</literal> lock.  The value of separating
+      Validation can be a long process on larger tables. The value of separating
       validation from initial creation is that you can defer validation to less
       busy times, or can be used to give additional time to correct pre-existing
-      errors while preventing new errors.
+      errors while preventing new errors. Note also that validation on its own
+      does not prevent normal write commands against the table while it runs.
+     </para>
+     <para>
+      Validation acquires only a <literal>SHARE UPDATE EXCLUSIVE</literal> lock
+      on the table being altered. If the constraint is a foreign key then
+      a <literal>ROW SHARE</literal> lock is also required on
+      the table referenced by the constraint.
      </para>
     </listitem>
    </varlistentry>
@@ -408,6 +424,9 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
       <xref linkend="SQL-CLUSTER">
       operations.  It does not actually re-cluster the table.
      </para>
+     <para>
+      Changing cluster options acquires a <literal>SHARE UPDATE EXCLUSIVE</literal> lock.
+     </para>
     </listitem>
    </varlistentry>
 
@@ -420,6 +439,9 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
       index specification from the table.  This affects
       future cluster operations that don't specify an index.
      </para>
+     <para>
+      Changing cluster options acquires a <literal>SHARE UPDATE EXCLUSIVE</literal> lock.
+     </para>
     </listitem>
    </varlistentry>
 
@@ -1078,6 +1100,14 @@ ALTER TABLE distributors ADD CONSTRAINT distfk FOREIGN KEY (address) REFERENCES
 </programlisting>
   </para>
 
+  <para>
+   To add a foreign key constraint to a table with the least impact on other work:
+<programlisting>
+ALTER TABLE distributors ADD CONSTRAINT distfk FOREIGN KEY (address) REFERENCES addresses (address) NOT VALID;
+ALTER TABLE distributors VALIDATE CONSTRAINT distfk;
+</programlisting>
+  </para>
+
   <para>
    To add a (multicolumn) unique constraint to a table:
 <programlisting>

src/backend/catalog/toasting.c

@@ -27,6 +27,7 @@
 #include "catalog/toasting.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
+#include "storage/lock.h"
 #include "utils/builtins.h"
 #include "utils/rel.h"
 #include "utils/syscache.h"
@@ -34,13 +35,15 @@
 /* Potentially set by contrib/pg_upgrade_support functions */
 Oid			binary_upgrade_next_toast_pg_type_oid = InvalidOid;
 
+static void CheckAndCreateToastTable(Oid relOid, Datum reloptions,
+					LOCKMODE lockmode, bool check);
 static bool create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
-				   Datum reloptions);
+					Datum reloptions, LOCKMODE lockmode, bool check);
 static bool needs_toast_table(Relation rel);
 
 
 /*
- * AlterTableCreateToastTable
+ * CreateToastTable variants
  *		If the table needs a toast table, and doesn't already have one,
  *		then create a toast table for it.
  *
@@ -52,21 +55,32 @@ static bool needs_toast_table(Relation rel);
  * to end with CommandCounterIncrement if it makes any changes.
  */
 void
-AlterTableCreateToastTable(Oid relOid, Datum reloptions)
+AlterTableCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode)
+{
+	CheckAndCreateToastTable(relOid, reloptions, lockmode, true);
+}
+
+void
+NewHeapCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode)
+{
+	CheckAndCreateToastTable(relOid, reloptions, lockmode, false);
+}
+
+void
+NewRelationCreateToastTable(Oid relOid, Datum reloptions)
+{
+	CheckAndCreateToastTable(relOid, reloptions, AccessExclusiveLock, false);
+}
+
+static void
+CheckAndCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode, bool check)
 {
 	Relation	rel;
 
-	/*
-	 * Grab an exclusive lock on the target table, since we'll update its
-	 * pg_class tuple. This is redundant for all present uses, since caller
-	 * will have such a lock already.  But the lock is needed to ensure that
-	 * concurrent readers of the pg_class tuple won't have visibility issues,
-	 * so let's be safe.
-	 */
-	rel = heap_open(relOid, AccessExclusiveLock);
+	rel = heap_open(relOid, lockmode);
 
 	/* create_toast_table does all the work */
-	(void) create_toast_table(rel, InvalidOid, InvalidOid, reloptions);
+	(void) create_toast_table(rel, InvalidOid, InvalidOid, reloptions, lockmode, check);
 
 	heap_close(rel, NoLock);
 }
@@ -91,7 +105,8 @@ BootstrapToastTable(char *relName, Oid toastOid, Oid toastIndexOid)
 						relName)));
 
 	/* create_toast_table does all the work */
-	if (!create_toast_table(rel, toastOid, toastIndexOid, (Datum) 0))
+	if (!create_toast_table(rel, toastOid, toastIndexOid, (Datum) 0,
+								AccessExclusiveLock, false))
 		elog(ERROR, "\"%s\" does not require a toast table",
 			 relName);
 
@@ -107,7 +122,8 @@ BootstrapToastTable(char *relName, Oid toastOid, Oid toastIndexOid)
  * bootstrap they can be nonzero to specify hand-assigned OIDs
  */
 static bool
-create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, Datum reloptions)
+create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
+				   Datum reloptions, LOCKMODE lockmode, bool check)
 {
 	Oid			relOid = RelationGetRelid(rel);
 	HeapTuple	reltup;
@@ -160,6 +176,13 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, Datum reloptio
 		 !OidIsValid(binary_upgrade_next_toast_pg_class_oid)))
 		return false;
 
+	/*
+	 * If requested check lockmode is sufficient. This is a cross check
+	 * in case of errors or conflicting decisions in earlier code.
+	 */
+	if (check && lockmode != AccessExclusiveLock)
+		elog(ERROR, "AccessExclusiveLock required to add toast table.");
+
 	/*
 	 * Create the toast table and its index
 	 */

src/backend/commands/cluster.c

@@ -408,10 +408,10 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
 /*
  * Verify that the specified heap and index are valid to cluster on
  *
- * Side effect: obtains exclusive lock on the index.  The caller should
- * already have exclusive lock on the table, so the index lock is likely
- * redundant, but it seems best to grab it anyway to ensure the index
- * definition can't change under us.
+ * Side effect: obtains lock on the index.  The caller may
+ * in some cases already have AccessExclusiveLock on the table, but
+ * not in all cases so we can't rely on the table-level lock for
+ * protection here.
  */
 void
 check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMODE lockmode)
@@ -696,10 +696,10 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
 	 *
 	 * If the relation doesn't have a TOAST table already, we can't need one
 	 * for the new relation.  The other way around is possible though: if some
-	 * wide columns have been dropped, AlterTableCreateToastTable can decide
+	 * wide columns have been dropped, NewHeapCreateToastTable can decide
 	 * that no TOAST table is needed for the new table.
 	 *
-	 * Note that AlterTableCreateToastTable ends with CommandCounterIncrement,
+	 * Note that NewHeapCreateToastTable ends with CommandCounterIncrement,
 	 * so that the TOAST table will be visible for insertion.
 	 */
 	toastid = OldHeap->rd_rel->reltoastrelid;
@@ -714,7 +714,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
 		if (isNull)
 			reloptions = (Datum) 0;
 
-		AlterTableCreateToastTable(OIDNewHeap, reloptions);
+		NewHeapCreateToastTable(OIDNewHeap, reloptions, lockmode);
 
 		ReleaseSysCache(tuple);
 	}

src/backend/commands/createas.c

@@ -359,7 +359,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 
 	/*
 	 * If necessary, create a TOAST table for the target table.  Note that
-	 * AlterTableCreateToastTable ends with CommandCounterIncrement(), so that
+	 * NewRelationCreateToastTable ends with CommandCounterIncrement(), so that
 	 * the TOAST table will be visible for insertion.
 	 */
 	CommandCounterIncrement();
@@ -373,7 +373,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 
 	(void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true);
 
-	AlterTableCreateToastTable(intoRelationId, toast_options);
+	NewRelationCreateToastTable(intoRelationId, toast_options);
 
 	/* Create the "view" part of a materialized view. */
 	if (is_matview)

src/backend/commands/tablecmds.c

@@ -2685,10 +2685,8 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
  * the whole operation; we don't have to do anything special to clean up.
  *
  * The caller must lock the relation, with an appropriate lock level
- * for the subcommands requested. Any subcommand that needs to rewrite
- * tuples in the table forces the whole command to be executed with
- * AccessExclusiveLock (actually, that is currently required always, but
- * we hope to relax it at some point).	We pass the lock level down
+ * for the subcommands requested, using AlterTableGetLockLevel(stmt->cmds)
+ * or higher. We pass the lock level down
  * so that we can apply it recursively to inherited tables. Note that the
  * lock level we want as we recurse might well be higher than required for
  * that specific subcommand. So we pass down the overall lock requirement,
@@ -2750,35 +2748,24 @@ AlterTableInternal(Oid relid, List *cmds, bool recurse)
  * ALTER TABLE RENAME which is treated as a different statement type T_RenameStmt
  * and does not travel through this section of code and cannot be combined with
  * any of the subcommands given here.
+ *
+ * Note that Hot Standby only knows about AccessExclusiveLocks on the master
+ * so any changes that might affect SELECTs running on standbys need to use
+ * AccessExclusiveLocks even if you think a lesser lock would do, unless you
+ * have a solution for that also.
+ *
+ * Also note that pg_dump uses only an AccessShareLock, meaning that anything
+ * that takes a lock less than AccessExclusiveLock can change object definitions
+ * while pg_dump is running. Be careful to check that the appropriate data is
+ * derived by pg_dump using an MVCC snapshot, rather than syscache lookups,
+ * otherwise we might end up with an inconsistent dump that can't restore.
  */
 LOCKMODE
 AlterTableGetLockLevel(List *cmds)
 {
 	/*
-	 * Late in 9.1 dev cycle a number of issues were uncovered with access to
-	 * catalog relations, leading to the decision to re-enforce all DDL at
-	 * AccessExclusiveLock level by default.
-	 *
-	 * The issues are that there is a pervasive assumption in the code that
-	 * the catalogs will not be read unless an AccessExclusiveLock is held. If
-	 * that rule is relaxed, we must protect against a number of potential
-	 * effects - infrequent, but proven possible with test cases where
-	 * multiple DDL operations occur in a stream against frequently accessed
-	 * tables.
-	 *
-	 * 1. Catalog tables were read using SnapshotNow, which has a race bug that
-	 * allows a scan to return no valid rows even when one is present in the
-	 * case of a commit of a concurrent update of the catalog table.
-	 * SnapshotNow also ignores transactions in progress, so takes the latest
-	 * committed version without waiting for the latest changes.
-	 *
-	 * 2. Relcache needs to be internally consistent, so unless we lock the
-	 * definition during reads we have no way to guarantee that.
-	 *
-	 * 3. Catcache access isn't coordinated at all so refreshes can occur at
-	 * any time.
+	 * This only works if we read catalog tables using MVCC snapshots.
 	 */
-#ifdef REDUCED_ALTER_TABLE_LOCK_LEVELS
 	ListCell   *lcmd;
 	LOCKMODE	lockmode = ShareUpdateExclusiveLock;
 
@@ -2790,28 +2777,58 @@ AlterTableGetLockLevel(List *cmds)
 		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.
+				 * These subcommands rewrite the heap, so require full locks.
 				 */
 			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_SetTableSpace:		/* must rewrite heap */
 			case AT_AlterColumnType:	/* must rewrite heap */
-			case AT_DropConstraint:		/* as DROP INDEX */
 			case AT_AddOids:			/* must rewrite heap */
+				cmd_lockmode = AccessExclusiveLock;
+				break;
+
+				/*
+				 * These subcommands may require addition of toast tables. If we
+				 * add a toast table to a table currently being scanned, we
+				 * might miss data added to the new toast table by concurrent
+				 * insert transactions.
+				 */
+			case AT_SetStorage:			/* may add toast tables, see ATRewriteCatalogs() */
+				cmd_lockmode = AccessExclusiveLock;
+				break;
+
+				/*
+				 * Removing constraints can affect SELECTs that have been
+				 * optimised assuming the constraint holds true.
+				 */
+			case AT_DropConstraint:		/* as DROP INDEX */
+			case AT_DropNotNull:		/* may change some SQL plans */
+				cmd_lockmode = AccessExclusiveLock;
+				break;
+
+				/*
+				 * Subcommands that may be visible to concurrent SELECTs
+				 */
+			case AT_DropColumn:			/* change visible to SELECT */
+			case AT_AddColumnToView:	/* CREATE VIEW */
 			case AT_DropOids:			/* calls AT_DropColumn */
 			case AT_EnableAlwaysRule:	/* may change SELECT rules */
 			case AT_EnableReplicaRule:	/* may change SELECT rules */
 			case AT_EnableRule:			/* may change SELECT rules */
 			case AT_DisableRule:		/* may change SELECT rules */
+				cmd_lockmode = AccessExclusiveLock;
+				break;
+
+				/*
+				 * Changing owner may remove implicit SELECT privileges
+				 */
 			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;
+
+				/*
+				 * Changing foreign table options may affect optimisation.
+				 */
 			case AT_GenericOptions:
 			case AT_AlterColumnGenericOptions:
 				cmd_lockmode = AccessExclusiveLock;
@@ -2819,6 +2836,7 @@ AlterTableGetLockLevel(List *cmds)
 
 				/*
 				 * These subcommands affect write operations only.
+				 * XXX Theoretically, these could be ShareRowExclusiveLock.
 				 */
 			case AT_ColumnDefault:
 			case AT_ProcessedConstraint:		/* becomes AT_AddConstraint */
@@ -2832,10 +2850,12 @@ AlterTableGetLockLevel(List *cmds)
 			case AT_DisableTrig:
 			case AT_DisableTrigAll:
 			case AT_DisableTrigUser:
+			case AT_AlterConstraint:
 			case AT_AddIndex:	/* from ADD CONSTRAINT */
 			case AT_AddIndexConstraint:
 			case AT_ReplicaIdentity:
-				cmd_lockmode = ShareRowExclusiveLock;
+			case AT_SetNotNull:
+				cmd_lockmode = AccessExclusiveLock;
 				break;
 
 			case AT_AddConstraint:
@@ -2854,8 +2874,10 @@ AlterTableGetLockLevel(List *cmds)
 							 * could reduce the lock strength to ShareLock if
 							 * we can work out how to allow concurrent catalog
 							 * updates.
+							 * XXX Might be set down to ShareRowExclusiveLock
+							 * but requires further analysis.
 							 */
-							cmd_lockmode = ShareRowExclusiveLock;
+							cmd_lockmode = AccessExclusiveLock;
 							break;
 						case CONSTR_FOREIGN:
 
@@ -2863,12 +2885,15 @@ 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
 							 */
-							cmd_lockmode = ShareRowExclusiveLock;
+							cmd_lockmode = AccessExclusiveLock;
 							break;
 
 						default:
-							cmd_lockmode = ShareRowExclusiveLock;
+							cmd_lockmode = AccessExclusiveLock;
 					}
 				}
 				break;
@@ -2879,22 +2904,31 @@ AlterTableGetLockLevel(List *cmds)
 				 * behaviour, while queries started after we commit will see
 				 * new behaviour. No need to prevent reads or writes to the
 				 * subtable while we hook it up though.
+				 * Changing the TupDesc may be a problem, so keep highest lock.
 				 */
 			case AT_AddInherit:
 			case AT_DropInherit:
-				cmd_lockmode = ShareUpdateExclusiveLock;
+				cmd_lockmode = AccessExclusiveLock;
 				break;
 
 				/*
 				 * These subcommands affect implicit row type conversion. They
-				 * have affects similar to CREATE/DROP CAST on queries.  We
+				 * have affects similar to CREATE/DROP CAST on queries.
 				 * don't provide for invalidating parse trees as a result of
-				 * such changes.  Do avoid concurrent pg_class updates,
-				 * though.
+				 * such changes, so we keep these at AccessExclusiveLock.
 				 */
 			case AT_AddOf:
 			case AT_DropOf:
-				cmd_lockmode = ShareUpdateExclusiveLock;
+				cmd_lockmode = AccessExclusiveLock;
+				break;
+
+				/*
+				 * Only used by CREATE OR REPLACE VIEW which must conflict
+				 * with an SELECTs currently using the view.
+				 */
+			case AT_ReplaceRelOptions:
+				cmd_lockmode = AccessExclusiveLock;
+				break;
 
 				/*
 				 * These subcommands affect general strategies for performance
@@ -2906,20 +2940,33 @@ AlterTableGetLockLevel(List *cmds)
 				 * applies: we don't currently allow concurrent catalog
 				 * updates.
 				 */
-			case AT_SetStatistics:
-			case AT_ClusterOn:
-			case AT_DropCluster:
-			case AT_SetRelOptions:
-			case AT_ResetRelOptions:
-			case AT_ReplaceRelOptions:
-			case AT_SetOptions:
-			case AT_ResetOptions:
-			case AT_SetStorage:
-			case AT_AlterConstraint:
-			case AT_ValidateConstraint:
+			case AT_SetStatistics:		/* Uses MVCC in getTableAttrs() */
+			case AT_ClusterOn:			/* Uses MVCC in getIndexes() */
+			case AT_DropCluster:		/* Uses MVCC in getIndexes() */
+			case AT_SetOptions:			/* Uses MVCC in getTableAttrs() */
+			case AT_ResetOptions:		/* Uses MVCC in getTableAttrs() */
 				cmd_lockmode = ShareUpdateExclusiveLock;
 				break;
 
+			case AT_ValidateConstraint:	/* Uses MVCC in getConstraints() */
+				cmd_lockmode = ShareUpdateExclusiveLock;
+				break;
+
+				/*
+				 * Rel options are more complex than first appears. Options
+				 * are set here for tables, views and indexes; for historical
+				 * reasons these can all be used with ALTER TABLE, so we
+				 * can't decide between them using the basic grammar.
+				 *
+				 * XXX Look in detail at each option to determine lock level,
+				 * e.g.
+				 *   cmd_lockmode = GetRelOptionsLockLevel((List *) cmd->def);
+				 */
+			case AT_SetRelOptions:		/* Uses MVCC in getIndexes() and getTables() */
+			case AT_ResetRelOptions:	/* Uses MVCC in getIndexes() and getTables() */
+				cmd_lockmode = AccessExclusiveLock;
+				break;
+
 			default:			/* oops */
 				elog(ERROR, "unrecognized alter table type: %d",
 					 (int) cmd->subtype);
@@ -2932,9 +2979,6 @@ AlterTableGetLockLevel(List *cmds)
 		if (cmd_lockmode > lockmode)
 			lockmode = cmd_lockmode;
 	}
-#else
-	LOCKMODE	lockmode = AccessExclusiveLock;
-#endif
 
 	return lockmode;
 }
@@ -3272,7 +3316,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
 
 		if (tab->relkind == RELKIND_RELATION ||
 			tab->relkind == RELKIND_MATVIEW)
-			AlterTableCreateToastTable(tab->relid, (Datum) 0);
+			AlterTableCreateToastTable(tab->relid, (Datum) 0, lockmode);
 	}
 }
 
@@ -3586,7 +3630,7 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode)
 
 			/* Create transient table that will receive the modified data */
 			OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, false,
-									   AccessExclusiveLock);
+									   lockmode);
 
 			/*
 			 * Copy the heap data into the new table with the desired

src/backend/tcop/utility.c

@@ -908,7 +908,7 @@ ProcessUtilitySlow(Node *parsetree,
 													InvalidOid);
 
 							/*
-							 * Let AlterTableCreateToastTable decide if this
+							 * Let NewRelationCreateToastTable decide if this
 							 * one needs a secondary relation too.
 							 */
 							CommandCounterIncrement();
@@ -927,7 +927,7 @@ ProcessUtilitySlow(Node *parsetree,
 												   toast_options,
 												   true);
 
-							AlterTableCreateToastTable(relOid, toast_options);
+							NewRelationCreateToastTable(relOid, toast_options);
 						}
 						else if (IsA(stmt, CreateForeignTableStmt))
 						{

src/backend/utils/adt/ruleutils.c

@@ -54,6 +54,7 @@
 #include "utils/fmgroids.h"
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
+#include "utils/snapmgr.h"
 #include "utils/syscache.h"
 #include "utils/tqual.h"
 #include "utils/typcache.h"
@@ -1284,6 +1285,9 @@ pg_get_constraintdef_string(Oid constraintId)
 	return pg_get_constraintdef_worker(constraintId, true, 0);
 }
 
+/*
+ * As of 9.4, we now use an MVCC snapshot for this.
+ */
 static char *
 pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
 							int prettyFlags)
@@ -1291,10 +1295,34 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
 	HeapTuple	tup;
 	Form_pg_constraint conForm;
 	StringInfoData buf;
+	SysScanDesc	scandesc;
+	ScanKeyData scankey[1];
+	Snapshot    snapshot = RegisterSnapshot(GetTransactionSnapshot());
+	Relation	relation = heap_open(ConstraintRelationId, AccessShareLock);
+
+	ScanKeyInit(&scankey[0],
+				ObjectIdAttributeNumber,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(constraintId));
+
+	scandesc = systable_beginscan(relation,
+									ConstraintOidIndexId,
+									true,
+									snapshot,
+									1,
+									scankey);
+
+	/*
+	 * We later use the tuple with SysCacheGetAttr() as if we
+	 * had obtained it via SearchSysCache, which works fine.
+	 */
+	tup = systable_getnext(scandesc);
+
+	UnregisterSnapshot(snapshot);
 
-	tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constraintId));
 	if (!HeapTupleIsValid(tup)) /* should not happen */
 		elog(ERROR, "cache lookup failed for constraint %u", constraintId);
+
 	conForm = (Form_pg_constraint) GETSTRUCT(tup);
 
 	initStringInfo(&buf);
@@ -1575,7 +1603,8 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
 		appendStringInfoString(&buf, " NOT VALID");
 
 	/* Cleanup */
-	ReleaseSysCache(tup);
+	systable_endscan(scandesc);
+	heap_close(relation, AccessShareLock);
 
 	return buf.data;
 }

src/backend/utils/cache/relcache.c

@@ -162,6 +162,14 @@ static bool eoxact_list_overflowed = false;
 			eoxact_list_overflowed = true; \
 	} while (0)
 
+/*
+ * EOXactTupleDescArray stores TupleDescs that (might) need AtEOXact
+ * cleanup work.  The array expands as needed; there is no hashtable because
+ * we don't need to access individual items except at EOXact.
+ */
+static TupleDesc *EOXactTupleDescArray;
+static int NextEOXactTupleDescNum = 0;
+static int EOXactTupleDescArrayLen = 0;
 
 /*
  *		macros to manipulate the lookup hashtables
@@ -220,11 +228,12 @@ static HTAB *OpClassCache = NULL;
 
 /* non-export function prototypes */
 
-static void RelationDestroyRelation(Relation relation);
+static void RelationDestroyRelation(Relation relation, bool remember_tupdesc);
 static void RelationClearRelation(Relation relation, bool rebuild);
 
 static void RelationReloadIndexInfo(Relation relation);
 static void RelationFlushRelation(Relation relation);
+static void RememberToFreeTupleDescAtEOX(TupleDesc td);
 static void AtEOXact_cleanup(Relation relation, bool isCommit);
 static void AtEOSubXact_cleanup(Relation relation, bool isCommit,
 					SubTransactionId mySubid, SubTransactionId parentSubid);
@@ -1858,7 +1867,7 @@ RelationReloadIndexInfo(Relation relation)
  *	Caller must already have unhooked the entry from the hash table.
  */
 static void
-RelationDestroyRelation(Relation relation)
+RelationDestroyRelation(Relation relation, bool remember_tupdesc)
 {
 	Assert(RelationHasReferenceCountZero(relation));
 
@@ -1878,7 +1887,20 @@ RelationDestroyRelation(Relation relation)
 	/* can't use DecrTupleDescRefCount here */
 	Assert(relation->rd_att->tdrefcount > 0);
 	if (--relation->rd_att->tdrefcount == 0)
+	{
+		/*
+		 * If we Rebuilt a relcache entry during a transaction then its
+		 * possible we did that because the TupDesc changed as the result
+		 * of an ALTER TABLE that ran at less than AccessExclusiveLock.
+		 * It's possible someone copied that TupDesc, in which case the
+		 * copy would point to free'd memory. So if we rebuild an entry
+		 * we keep the TupDesc around until end of transaction, to be safe.
+		 */
+		if (remember_tupdesc)
+			RememberToFreeTupleDescAtEOX(relation->rd_att);
+		else
 			FreeTupleDesc(relation->rd_att);
+	}
 	list_free(relation->rd_indexlist);
 	bms_free(relation->rd_indexattr);
 	FreeTriggerDesc(relation->trigdesc);
@@ -1992,7 +2014,7 @@ RelationClearRelation(Relation relation, bool rebuild)
 		RelationCacheDelete(relation);
 
 		/* And release storage */
-		RelationDestroyRelation(relation);
+		RelationDestroyRelation(relation, false);
 	}
 	else if (!IsTransactionState())
 	{
@@ -2059,7 +2081,7 @@ RelationClearRelation(Relation relation, bool rebuild)
 		{
 			/* Should only get here if relation was deleted */
 			RelationCacheDelete(relation);
-			RelationDestroyRelation(relation);
+			RelationDestroyRelation(relation, false);
 			elog(ERROR, "relation %u deleted while still in use", save_relid);
 		}
 
@@ -2121,7 +2143,7 @@ RelationClearRelation(Relation relation, bool rebuild)
 #undef SWAPFIELD
 
 		/* And now we can throw away the temporary entry */
-		RelationDestroyRelation(newrel);
+		RelationDestroyRelation(newrel, !keep_tupdesc);
 	}
 }
 
@@ -2359,6 +2381,33 @@ RelationCloseSmgrByOid(Oid relationId)
 	RelationCloseSmgr(relation);
 }
 
+void
+RememberToFreeTupleDescAtEOX(TupleDesc td)
+{
+	if (EOXactTupleDescArray == NULL)
+	{
+		MemoryContext	oldcxt;
+		oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
+
+		EOXactTupleDescArray = (TupleDesc *) palloc(16 * sizeof(TupleDesc));
+		EOXactTupleDescArrayLen = 16;
+		NextEOXactTupleDescNum = 0;
+		MemoryContextSwitchTo(oldcxt);
+	}
+	else if (NextEOXactTupleDescNum >= EOXactTupleDescArrayLen)
+	{
+		int32       	newlen = EOXactTupleDescArrayLen * 2;
+
+		Assert(EOXactTupleDescArrayLen > 0);
+
+		EOXactTupleDescArray = (TupleDesc *) repalloc(EOXactTupleDescArray,
+														newlen * sizeof(TupleDesc));
+		EOXactTupleDescArrayLen = newlen;
+	}
+
+	EOXactTupleDescArray[NextEOXactTupleDescNum++] = td;
+}
+
 /*
  * AtEOXact_RelationCache
  *
@@ -2414,9 +2463,20 @@ AtEOXact_RelationCache(bool isCommit)
 		}
 	}
 
-	/* Now we're out of the transaction and can clear the list */
+	if (EOXactTupleDescArrayLen > 0)
+	{
+		Assert(EOXactTupleDescArray != NULL);
+		for (i = 0; i < NextEOXactTupleDescNum; i++)
+			FreeTupleDesc(EOXactTupleDescArray[i]);
+		pfree(EOXactTupleDescArray);
+		EOXactTupleDescArray = NULL;
+	}
+
+	/* Now we're out of the transaction and can clear the lists */
 	eoxact_list_len = 0;
 	eoxact_list_overflowed = false;
+	NextEOXactTupleDescNum = 0;
+	EOXactTupleDescArrayLen = 0;
 }
 
 /*

src/include/catalog/toasting.h

@@ -14,10 +14,16 @@
 #ifndef TOASTING_H
 #define TOASTING_H
 
+#include "storage/lock.h"
+
 /*
  * toasting.c prototypes
  */
-extern void AlterTableCreateToastTable(Oid relOid, Datum reloptions);
+extern void NewRelationCreateToastTable(Oid relOid, Datum reloptions);
+extern void NewHeapCreateToastTable(Oid relOid, Datum reloptions,
+									LOCKMODE lockmode);
+extern void AlterTableCreateToastTable(Oid relOid, Datum reloptions,
+									LOCKMODE lockmode);
 extern void BootstrapToastTable(char *relName,
 					Oid toastOid, Oid toastIndexOid);
 

src/test/isolation/isolation_schedule

@@ -23,4 +23,5 @@ test: multixact-no-deadlock
 test: multixact-no-forget
 test: propagate-lock-delete
 test: drop-index-concurrently-1
+test: alter-table-1
 test: timeouts

src/test/regress/expected/alter_table.out

@@ -1840,28 +1840,31 @@ and relnamespace != (select oid from pg_namespace where nspname = 'pg_catalog')
 and c.relname != 'my_locks'
 group by c.relname;
 create table alterlock (f1 int primary key, f2 text);
+insert into alterlock values (1, 'foo');
+create table alterlock2 (f3 int primary key, f1 int);
+insert into alterlock2 values (1, 1);
 begin; alter table alterlock alter column f2 set statistics 150;
 select * from my_locks order by 1;
   relname  |       max_lockmode       
------------+---------------------
- alterlock | AccessExclusiveLock
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
 (1 row)
 
 rollback;
 begin; alter table alterlock cluster on alterlock_pkey;
 select * from my_locks order by 1;
     relname     |       max_lockmode       
-----------------+---------------------
- alterlock      | AccessExclusiveLock
- alterlock_pkey | AccessExclusiveLock
+----------------+--------------------------
+ alterlock      | ShareUpdateExclusiveLock
+ alterlock_pkey | ShareUpdateExclusiveLock
 (2 rows)
 
 commit;
 begin; alter table alterlock set without cluster;
 select * from my_locks order by 1;
   relname  |       max_lockmode       
------------+---------------------
- alterlock | AccessExclusiveLock
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
 (1 row)
 
 commit;
@@ -1904,8 +1907,8 @@ commit;
 begin; alter table alterlock alter column f2 set (n_distinct = 1);
 select * from my_locks order by 1;
   relname  |       max_lockmode       
------------+---------------------
- alterlock | AccessExclusiveLock
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
 (1 row)
 
 rollback;
@@ -1924,8 +1927,62 @@ select * from my_locks order by 1;
  alterlock | AccessExclusiveLock
 (1 row)
 
+rollback;
+begin;
+create trigger ttdummy
+	before delete or update on alterlock
+	for each row
+	execute procedure
+	ttdummy (1, 1);
+select * from my_locks order by 1;
+  relname  |    max_lockmode     
+-----------+---------------------
+ alterlock | AccessExclusiveLock
+(1 row)
+
+rollback;
+begin;
+select * from my_locks order by 1;
+ relname | max_lockmode 
+---------+--------------
+(0 rows)
+
+alter table alterlock2 add foreign key (f1) references alterlock (f1);
+select * from my_locks order by 1;
+     relname     |    max_lockmode     
+-----------------+---------------------
+ alterlock       | AccessExclusiveLock
+ alterlock2      | AccessExclusiveLock
+ alterlock2_pkey | AccessShareLock
+ alterlock_pkey  | AccessShareLock
+(4 rows)
+
+rollback;
+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
+(2 rows)
+
+commit;
+begin;
+alter table alterlock2 validate constraint alterlock2nv;
+select * from my_locks order by 1;
+     relname     |       max_lockmode       
+-----------------+--------------------------
+ alterlock       | RowShareLock
+ alterlock2      | ShareUpdateExclusiveLock
+ alterlock2_pkey | AccessShareLock
+ alterlock_pkey  | AccessShareLock
+(4 rows)
+
 rollback;
 -- cleanup
+drop table alterlock2;
 drop table alterlock;
 drop view my_locks;
 drop type lockmodes;

src/test/regress/sql/alter_table.sql

@@ -1283,6 +1283,9 @@ and c.relname != 'my_locks'
 group by c.relname;
 
 create table alterlock (f1 int primary key, f2 text);
+insert into alterlock values (1, 'foo');
+create table alterlock2 (f3 int primary key, f1 int);
+insert into alterlock2 values (1, 1);
 
 begin; alter table alterlock alter column f2 set statistics 150;
 select * from my_locks order by 1;
@@ -1324,7 +1327,33 @@ begin; alter table alterlock alter column f2 set default 'x';
 select * from my_locks order by 1;
 rollback;
 
+begin;
+create trigger ttdummy
+	before delete or update on alterlock
+	for each row
+	execute procedure
+	ttdummy (1, 1);
+select * from my_locks order by 1;
+rollback;
+
+begin;
+select * from my_locks order by 1;
+alter table alterlock2 add foreign key (f1) references alterlock (f1);
+select * from my_locks order by 1;
+rollback;
+
+begin;
+alter table alterlock2
+add constraint alterlock2nv foreign key (f1) references alterlock (f1) NOT VALID;
+select * from my_locks order by 1;
+commit;
+begin;
+alter table alterlock2 validate constraint alterlock2nv;
+select * from my_locks order by 1;
+rollback;
+
 -- cleanup
+drop table alterlock2;
 drop table alterlock;
 drop view my_locks;
 drop type lockmodes;