Improve error message for rejecting RETURNING clauses with dropped columns.

tglsfdc · tglsfdc · commit 55965eb7d2f2 · 2016-02-29T19:11:49.000-05:00
This error message was written with only ON SELECT rules in mind, but since
then we also made RETURNING-clause targetlists go through the same logic.
This means that you got a rather off-topic error message if you tried to
add a rule with RETURNING to a table having dropped columns.  Ideally we'd
just support that, but some preliminary investigation says that it might be
a significant amount of work.  Seeing that Nicklas Avén's complaint is the
first one we've gotten about this in the ten years or so that the code's
been like that, I'm unwilling to put much time into it.  Instead, improve
the error report by issuing a different message for RETURNING cases, and
revise the associated comment based on this investigation.

Discussion: 1456176604.17219.9.camel@jordogskog.no
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
@@ -654,17 +654,29 @@ checkRuleResultList(List *targetList, TupleDesc resultDesc, bool isSelect,
 		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.
+		 * Disallow dropped columns in the relation.  This is not really
+		 * expected to happen when creating an ON SELECT rule.  It'd be
+		 * possible if someone tried to convert a relation with dropped
+		 * columns to a view, but the only case we care about supporting
+		 * table-to-view conversion for is pg_dump, and pg_dump won't do that.
+		 *
+		 * Unfortunately, the situation is also possible when adding a rule
+		 * with RETURNING to a regular table, and rejecting that case is
+		 * altogether more annoying.  In principle we could support it by
+		 * modifying the targetlist to include dummy NULL columns
+		 * corresponding to the dropped columns in the tupdesc.  However,
+		 * places like ruleutils.c would have to be fixed to not process such
+		 * entries, and that would take an uncertain and possibly rather large
+		 * amount of work.  (Note we could not dodge that by marking the dummy
+		 * columns resjunk, since it's precisely the non-resjunk tlist columns
+		 * that are expected to correspond to table columns.)
 		 */
 		if (attr->attisdropped)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot convert relation containing dropped columns to view")));
+					 isSelect ?
+					 errmsg("cannot convert relation containing dropped columns to view") :
+					 errmsg("cannot create a RETURNING list for a relation containing dropped columns")));
 
 		/* Check name match if required; no need for two error texts here */
 		if (requireColumnNameMatch && strcmp(tle->resname, attname) != 0)
