Apply a simple solution to the problem of making INSERT/UPDATE/DELETE

RETURNING play nice with views/rules.  To wit, have the rule rewriter
rewrite any RETURNING clause found in a rule to produce what the rule's
triggering query asked for in its RETURNING clause, in particular drop
the RETURNING clause if no RETURNING in the triggering query.  This
leaves the responsibility for knowing how to produce the view's output
columns on the rule author, without requiring any fundamental changes
in rule semantics such as adding new rule event types would do.  The
initial implementation constrains things to ensure that there is
exactly one, unconditionally invoked RETURNING clause among the rules
for an event --- later we might be able to relax that, but for a post
feature freeze fix it seems better to minimize how much invention we do.
Per gripe from Jaime Casanova.

2 files changed, 226 insertions, 70 deletions

diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index df24c6751c..a961f49d9b 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/rewrite/rewriteDefine.c,v 1.112 2006/08/12 20:05:55 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/rewrite/rewriteDefine.c,v 1.113 2006/09/02 17:06:52 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -31,6 +31,8 @@
 #include "utils/syscache.h"
 
 
+static void checkRuleResultList(List *targetList, TupleDesc resultDesc,
+								bool isSelect);
 static void setRuleCheckAsUser_Query(Query *qry, Oid userid);
 static void setRuleCheckAsUser_Expr(Node *node, Oid userid);
 static bool setRuleCheckAsUser_walker(Node *node, Oid *context);
@@ -235,15 +237,11 @@ DefineQueryRewrite(RuleStmt *stmt)
 					 errhint("Use triggers instead.")));
 	}
 
-	/*
-	 * Rules ON SELECT are restricted to view definitions
-	 */
 	if (event_type == CMD_SELECT)
 	{
-		ListCell   *tllist;
-		int			i;
-
 		/*
+		 * Rules ON SELECT are restricted to view definitions
+		 *
 		 * So there cannot be INSTEAD NOTHING, ...
 		 */
 		if (list_length(action) == 0)
@@ -282,71 +280,17 @@ DefineQueryRewrite(RuleStmt *stmt)
 		 * ... the targetlist of the SELECT action must exactly match the
 		 * event relation, ...
 		 */
-		i = 0;
-		foreach(tllist, query->targetList)
-		{
-			TargetEntry *tle = (TargetEntry *) lfirst(tllist);
-			int32		tletypmod;
-			Form_pg_attribute attr;
-			char	   *attname;
-
-			if (tle->resjunk)
-				continue;
-			i++;
-			if (i > event_relation->rd_att->natts)
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-				  errmsg("SELECT rule's target list has too many entries")));
-
-			attr = event_relation->rd_att->attrs[i - 1];
-			attname = NameStr(attr->attname);
-
-			/*
-			 * Disallow dropped columns in the relation.  This won't happen in
-			 * the cases we actually care about (namely creating a view via
-			 * CREATE TABLE then CREATE RULE).	Trying to cope with it is much
-			 * more trouble than it's worth, because we'd have to modify the
-			 * rule to insert dummy NULLs at the right positions.
-			 */
-			if (attr->attisdropped)
-				ereport(ERROR,
-						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-						 errmsg("cannot convert relation containing dropped columns to view")));
-
-			if (strcmp(tle->resname, attname) != 0)
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-						 errmsg("SELECT rule's target entry %d has different column name from \"%s\"", i, attname)));
-
-			if (attr->atttypid != exprType((Node *) tle->expr))
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-						 errmsg("SELECT rule's target entry %d has different type from column \"%s\"", i, attname)));
-
-			/*
-			 * Allow typmods to be different only if one of them is -1, ie,
-			 * "unspecified".  This is necessary for cases like "numeric",
-			 * where the table will have a filled-in default length but the
-			 * select rule's expression will probably have typmod = -1.
-			 */
-			tletypmod = exprTypmod((Node *) tle->expr);
-			if (attr->atttypmod != tletypmod &&
-				attr->atttypmod != -1 && tletypmod != -1)
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-						 errmsg("SELECT rule's target entry %d has different size from column \"%s\"", i, attname)));
-		}
-
-		if (i != event_relation->rd_att->natts)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-				   errmsg("SELECT rule's target list has too few entries")));
+		checkRuleResultList(query->targetList,
+							RelationGetDescr(event_relation),
+							true);
 
 		/*
 		 * ... there must not be another ON SELECT rule already ...
 		 */
 		if (!replace && event_relation->rd_rules != NULL)
 		{
+			int		i;
+
 			for (i = 0; i < event_relation->rd_rules->numLocks; i++)
 			{
 				RewriteRule *rule;
@@ -425,6 +369,42 @@ DefineQueryRewrite(RuleStmt *stmt)
 			RelisBecomingView = true;
 		}
 	}
+	else
+	{
+		/*
+		 * For non-SELECT rules, a RETURNING list can appear in at most one
+		 * of the actions ... and there can't be any RETURNING list at all
+		 * in a conditional or non-INSTEAD rule.  (Actually, there can be
+		 * at most one RETURNING list across all rules on the same event,
+		 * but it seems best to enforce that at rule expansion time.)  If
+		 * there is a RETURNING list, it must match the event relation.
+		 */
+		bool	haveReturning = false;
+
+		foreach(l, action)
+		{
+			query = (Query *) lfirst(l);
+
+			if (!query->returningList)
+				continue;
+			if (haveReturning)
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("cannot have multiple RETURNING lists in a rule")));
+			haveReturning = true;
+			if (event_qual != NULL)
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("RETURNING lists are not supported in conditional rules")));
+			if (!is_instead)
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("RETURNING lists are not supported in non-INSTEAD rules")));
+			checkRuleResultList(query->returningList,
+								RelationGetDescr(event_relation),
+								false);
+		}
+	}
 
 	/*
 	 * This rule is allowed - prepare to install it.
@@ -485,6 +465,95 @@ DefineQueryRewrite(RuleStmt *stmt)
 }
 
 /*
+ * checkRuleResultList
+ *		Verify that targetList produces output compatible with a tupledesc
+ *
+ * The targetList might be either a SELECT targetlist, or a RETURNING list;
+ * isSelect tells which.  (This is mostly used for choosing error messages,
+ * but also we don't enforce column name matching for RETURNING.)
+ */
+static void
+checkRuleResultList(List *targetList, TupleDesc resultDesc, bool isSelect)
+{
+	ListCell   *tllist;
+	int			i;
+
+	i = 0;
+	foreach(tllist, targetList)
+	{
+		TargetEntry *tle = (TargetEntry *) lfirst(tllist);
+		int32		tletypmod;
+		Form_pg_attribute attr;
+		char	   *attname;
+
+		/* resjunk entries may be ignored */
+		if (tle->resjunk)
+			continue;
+		i++;
+		if (i > resultDesc->natts)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+					 isSelect ?
+					 errmsg("SELECT rule's target list has too many entries") :
+					 errmsg("RETURNING list has too many entries")));
+
+		attr = resultDesc->attrs[i - 1];
+		attname = NameStr(attr->attname);
+
+		/*
+		 * Disallow dropped columns in the relation.  This won't happen in the
+		 * cases we actually care about (namely creating a view via CREATE
+		 * TABLE then CREATE RULE, or adding a RETURNING rule to a view).
+		 * Trying to cope with it is much more trouble than it's worth,
+		 * because we'd have to modify the rule to insert dummy NULLs at the
+		 * right positions.
+		 */
+		if (attr->attisdropped)
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("cannot convert relation containing dropped columns to view")));
+
+		if (isSelect && strcmp(tle->resname, attname) != 0)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+					 errmsg("SELECT rule's target entry %d has different column name from \"%s\"", i, attname)));
+
+		if (attr->atttypid != exprType((Node *) tle->expr))
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+					 isSelect ?
+					 errmsg("SELECT rule's target entry %d has different type from column \"%s\"",
+							i, attname) :
+					 errmsg("RETURNING list's entry %d has different type from column \"%s\"",
+							i, attname)));
+
+		/*
+		 * Allow typmods to be different only if one of them is -1, ie,
+		 * "unspecified".  This is necessary for cases like "numeric",
+		 * where the table will have a filled-in default length but the
+		 * select rule's expression will probably have typmod = -1.
+		 */
+		tletypmod = exprTypmod((Node *) tle->expr);
+		if (attr->atttypmod != tletypmod &&
+			attr->atttypmod != -1 && tletypmod != -1)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+					 isSelect ?
+					 errmsg("SELECT rule's target entry %d has different size from column \"%s\"",
+							i, attname) :
+					 errmsg("RETURNING list's entry %d has different size from column \"%s\"",
+							i, attname)));
+	}
+
+	if (i != resultDesc->natts)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+				 isSelect ?
+				 errmsg("SELECT rule's target list has too few entries") :
+				 errmsg("RETURNING list has too few entries")));
+}
+
+/*
  * setRuleCheckAsUser_Query
  *		Recursively scan a query and set the checkAsUser field to the
  *		given userid in all rtable entries.
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 286fc5b498..1af5bd7e7f 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/rewrite/rewriteHandler.c,v 1.165 2006/08/02 01:59:47 joe Exp $
+ *	  $PostgreSQL: pgsql/src/backend/rewrite/rewriteHandler.c,v 1.166 2006/09/02 17:06:52 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -39,7 +39,8 @@ static Query *rewriteRuleAction(Query *parsetree,
 				  Query *rule_action,
 				  Node *rule_qual,
 				  int rt_index,
-				  CmdType event);
+				  CmdType event,
+				  bool *returning_flag);
 static List *adjustJoinTreeList(Query *parsetree, bool removert, int rt_index);
 static void rewriteTargetList(Query *parsetree, Relation target_relation,
 							  List **attrno_list);
@@ -251,13 +252,26 @@ acquireLocksOnSubLinks(Node *node, void *context)
  * rewriteRuleAction -
  *	  Rewrite the rule action with appropriate qualifiers (taken from
  *	  the triggering query).
+ *
+ * Input arguments:
+ *	parsetree - original query
+ *	rule_action - one action (query) of a rule
+ *	rule_qual - WHERE condition of rule, or NULL if unconditional
+ *	rt_index - RT index of result relation in original query
+ *	event - type of rule event
+ * Output arguments:
+ *	*returning_flag - set TRUE if we rewrite RETURNING clause in rule_action
+ *					(must be initialized to FALSE)
+ * Return value:
+ *	rewritten form of rule_action
  */
 static Query *
 rewriteRuleAction(Query *parsetree,
 				  Query *rule_action,
 				  Node *rule_qual,
 				  int rt_index,
-				  CmdType event)
+				  CmdType event,
+				  bool *returning_flag)
 {
 	int			current_varno,
 				new_varno;
@@ -416,6 +430,32 @@ rewriteRuleAction(Query *parsetree,
 			rule_action = sub_action;
 	}
 
+	/*
+	 * If rule_action has a RETURNING clause, then either throw it away
+	 * if the triggering query has no RETURNING clause, or rewrite it to
+	 * emit what the triggering query's RETURNING clause asks for.  Throw
+	 * an error if more than one rule has a RETURNING clause.
+	 */
+	if (!parsetree->returningList)
+		rule_action->returningList = NIL;
+	else if (rule_action->returningList)
+	{
+		if (*returning_flag)
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("cannot have RETURNING lists in multiple rules")));
+		*returning_flag = true;
+		rule_action->returningList = (List *)
+			ResolveNew((Node *) parsetree->returningList,
+					   parsetree->resultRelation,
+					   0,
+					   rt_fetch(parsetree->resultRelation,
+								parsetree->rtable),
+					   rule_action->returningList,
+					   CMD_SELECT,
+					   0);
+	}
+
 	return rule_action;
 }
 
@@ -1357,6 +1397,8 @@ CopyAndAddInvertedQual(Query *parsetree,
  * Output arguments:
  *	*instead_flag - set TRUE if any unqualified INSTEAD rule is found
  *					(must be initialized to FALSE)
+ *	*returning_flag - set TRUE if we rewrite RETURNING clause in any rule
+ *					(must be initialized to FALSE)
  *	*qual_product - filled with modified original query if any qualified
  *					INSTEAD rule is found (must be initialized to NULL)
  * Return value:
@@ -1377,6 +1419,7 @@ fireRules(Query *parsetree,
 		  CmdType event,
 		  List *locks,
 		  bool *instead_flag,
+		  bool *returning_flag,
 		  Query **qual_product)
 {
 	List	   *results = NIL;
@@ -1438,7 +1481,8 @@ fireRules(Query *parsetree,
 				continue;
 
 			rule_action = rewriteRuleAction(parsetree, rule_action,
-											event_qual, rt_index, event);
+											event_qual, rt_index, event,
+											returning_flag);
 
 			rule_action->querySource = qsrc;
 			rule_action->canSetTag = false;		/* might change later */
@@ -1463,6 +1507,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
 {
 	CmdType		event = parsetree->commandType;
 	bool		instead = false;
+	bool		returning = false;
 	Query	   *qual_product = NULL;
 	List	   *rewritten = NIL;
 
@@ -1551,6 +1596,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
 										event,
 										locks,
 										&instead,
+										&returning,
 										&qual_product);
 
 			/*
@@ -1591,6 +1637,47 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
 			}
 		}
 
+		/*
+		 * If there is an INSTEAD, and the original query has a RETURNING,
+		 * we have to have found a RETURNING in the rule(s), else fail.
+		 * (Because DefineQueryRewrite only allows RETURNING in unconditional
+		 * INSTEAD rules, there's no need to worry whether the substituted
+		 * RETURNING will actually be executed --- it must be.)
+		 */
+		if ((instead || qual_product != NULL) &&
+			parsetree->returningList &&
+			!returning)
+		{
+			switch (event)
+			{
+				case CMD_INSERT:
+					ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("cannot INSERT RETURNING on relation \"%s\"",
+									RelationGetRelationName(rt_entry_relation)),
+							 errhint("You need an unconditional ON INSERT DO INSTEAD rule with a RETURNING clause.")));
+					break;
+				case CMD_UPDATE:
+					ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("cannot UPDATE RETURNING on relation \"%s\"",
+									RelationGetRelationName(rt_entry_relation)),
+							 errhint("You need an unconditional ON UPDATE DO INSTEAD rule with a RETURNING clause.")));
+					break;
+				case CMD_DELETE:
+					ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("cannot DELETE RETURNING on relation \"%s\"",
+									RelationGetRelationName(rt_entry_relation)),
+							 errhint("You need an unconditional ON DELETE DO INSTEAD rule with a RETURNING clause.")));
+					break;
+				default:
+					elog(ERROR, "unrecognized commandType: %d",
+						 (int) event);
+					break;
+			}
+		}
+
 		heap_close(rt_entry_relation, NoLock);
 	}