Restructure ALTER TABLE execution to fix assorted bugs.

We've had numerous bug reports about how (1) IF NOT EXISTS clauses in
ALTER TABLE don't behave as-expected, and (2) combining certain actions
into one ALTER TABLE doesn't work, though executing the same actions as
separate statements does.  This patch cleans up all of the cases so far
reported from the field, though there are still some oddities associated
with identity columns.

The core problem behind all of these bugs is that we do parse analysis
of ALTER TABLE subcommands too soon, before starting execution of the
statement.  The root of the bugs in group (1) is that parse analysis
schedules derived commands (such as a CREATE SEQUENCE for a serial
column) before it's known whether the IF NOT EXISTS clause should cause
a subcommand to be skipped.  The root of the bugs in group (2) is that
earlier subcommands may change the catalog state that later subcommands
need to be parsed against.

Hence, postpone parse analysis of ALTER TABLE's subcommands, and do
that one subcommand at a time, during "phase 2" of ALTER TABLE which
is the phase that does catalog rewrites.  Thus the catalog effects
of earlier subcommands are already visible when we analyze later ones.
(The sole exception is that we do parse analysis for ALTER COLUMN TYPE
subcommands during phase 1, so that their USING expressions can be
parsed against the table's original state, which is what we need.
Arguably, these bugs stem from falsely concluding that because ALTER
COLUMN TYPE must do early parse analysis, every other command subtype
can too.)

This means that ALTER TABLE itself must deal with execution of any
non-ALTER-TABLE derived statements that are generated by parse analysis.
Add a suitable entry point to utility.c to accept those recursive
calls, and create a struct to pass through the information needed by
the recursive call, rather than making the argument lists of
AlterTable() and friends even longer.

Getting this to work correctly required a little bit of fiddling
with the subcommand pass structure, in particular breaking up
AT_PASS_ADD_CONSTR into multiple passes.  But otherwise it's mostly
a pretty straightforward application of the above ideas.

Fixing the residual issues for identity columns requires refactoring of
where the dependency link from an identity column to its sequence gets
set up.  So that seems like suitable material for a separate patch,
especially since this one is pretty big already.

Discussion: https://postgr.es/m/10365.1558909428@sss.pgh.pa.us

1 files changed, 61 insertions, 54 deletions

diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index b2c58bf8622..a50e7ff4b43 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1093,8 +1093,6 @@ ProcessUtilitySlow(ParseState *pstate,
 				{
 					AlterTableStmt *atstmt = (AlterTableStmt *) parsetree;
 					Oid			relid;
-					List	   *stmts;
-					ListCell   *l;
 					LOCKMODE	lockmode;
 
 					/*
@@ -1108,59 +1106,21 @@ ProcessUtilitySlow(ParseState *pstate,
 
 					if (OidIsValid(relid))
 					{
-						/* Run parse analysis ... */
-						stmts = transformAlterTableStmt(relid, atstmt,
-														queryString);
+						AlterTableUtilityContext atcontext;
+
+						/* Set up info needed for recursive callbacks ... */
+						atcontext.pstmt = pstmt;
+						atcontext.queryString = queryString;
+						atcontext.relid = relid;
+						atcontext.params = params;
+						atcontext.queryEnv = queryEnv;
 
 						/* ... ensure we have an event trigger context ... */
 						EventTriggerAlterTableStart(parsetree);
 						EventTriggerAlterTableRelid(relid);
 
 						/* ... and do it */
-						foreach(l, stmts)
-						{
-							Node	   *stmt = (Node *) lfirst(l);
-
-							if (IsA(stmt, AlterTableStmt))
-							{
-								/* Do the table alteration proper */
-								AlterTable(relid, lockmode,
-										   (AlterTableStmt *) stmt);
-							}
-							else
-							{
-								/*
-								 * Recurse for anything else.  If we need to
-								 * do so, "close" the current complex-command
-								 * set, and start a new one at the bottom;
-								 * this is needed to ensure the ordering of
-								 * queued commands is consistent with the way
-								 * they are executed here.
-								 */
-								PlannedStmt *wrapper;
-
-								EventTriggerAlterTableEnd();
-								wrapper = makeNode(PlannedStmt);
-								wrapper->commandType = CMD_UTILITY;
-								wrapper->canSetTag = false;
-								wrapper->utilityStmt = stmt;
-								wrapper->stmt_location = pstmt->stmt_location;
-								wrapper->stmt_len = pstmt->stmt_len;
-								ProcessUtility(wrapper,
-											   queryString,
-											   PROCESS_UTILITY_SUBCOMMAND,
-											   params,
-											   NULL,
-											   None_Receiver,
-											   NULL);
-								EventTriggerAlterTableStart(parsetree);
-								EventTriggerAlterTableRelid(relid);
-							}
-
-							/* Need CCI between commands */
-							if (lnext(stmts, l) != NULL)
-								CommandCounterIncrement();
-						}
+						AlterTable(atstmt, lockmode, &atcontext);
 
 						/* done */
 						EventTriggerAlterTableEnd();
@@ -1718,6 +1678,52 @@ ProcessUtilitySlow(ParseState *pstate,
 }
 
 /*
+ * ProcessUtilityForAlterTable
+ *		Recursive entry from ALTER TABLE
+ *
+ * ALTER TABLE sometimes generates subcommands such as CREATE INDEX.
+ * It calls this, not the main entry point ProcessUtility, to execute
+ * such subcommands.
+ *
+ * stmt: the utility command to execute
+ * context: opaque passthrough struct with the info we need
+ *
+ * It's caller's responsibility to do CommandCounterIncrement after
+ * calling this, if needed.
+ */
+void
+ProcessUtilityForAlterTable(Node *stmt, AlterTableUtilityContext *context)
+{
+	PlannedStmt *wrapper;
+
+	/*
+	 * For event triggers, we must "close" the current complex-command set,
+	 * and start a new one afterwards; this is needed to ensure the ordering
+	 * of command events is consistent with the way they were executed.
+	 */
+	EventTriggerAlterTableEnd();
+
+	/* Create a suitable wrapper */
+	wrapper = makeNode(PlannedStmt);
+	wrapper->commandType = CMD_UTILITY;
+	wrapper->canSetTag = false;
+	wrapper->utilityStmt = stmt;
+	wrapper->stmt_location = context->pstmt->stmt_location;
+	wrapper->stmt_len = context->pstmt->stmt_len;
+
+	ProcessUtility(wrapper,
+				   context->queryString,
+				   PROCESS_UTILITY_SUBCOMMAND,
+				   context->params,
+				   context->queryEnv,
+				   None_Receiver,
+				   NULL);
+
+	EventTriggerAlterTableStart(context->pstmt->utilityStmt);
+	EventTriggerAlterTableRelid(context->relid);
+}
+
+/*
  * Dispatch function for DropStmt
  */
 static void
@@ -2394,14 +2400,15 @@ CreateCommandTag(Node *parsetree)
 			break;
 
 		case T_RenameStmt:
+
 			/*
-			 * When the column is renamed, the command tag is created
-			 * from its relation type
+			 * When the column is renamed, the command tag is created from its
+			 * relation type
 			 */
 			tag = AlterObjectTypeCommandTag(
-				((RenameStmt *) parsetree)->renameType == OBJECT_COLUMN ?
-				((RenameStmt *) parsetree)->relationType :
-				((RenameStmt *) parsetree)->renameType);
+											((RenameStmt *) parsetree)->renameType == OBJECT_COLUMN ?
+											((RenameStmt *) parsetree)->relationType :
+											((RenameStmt *) parsetree)->renameType);
 			break;
 
 		case T_AlterObjectDependsStmt: