Fix reporting of column typmods for multi-row VALUES constructs.

tglsfdc · tglsfdc · commit 2afe282a3732 · 2016-12-09T12:01:14.000-05:00
expandRTE() and get_rte_attribute_type() reported the exprType() and exprTypmod() values of the expressions in the first row of the VALUES as being the column type/typmod returned by the VALUES RTE. That's fine for the data type, since we coerce all expressions in a column to have the same common type. But we don't coerce them to have a common typmod, so it was possible for rows after the first one to return values that violate the claimed column typmod. This leads to the incorrect result seen in bug #14448 from Hassan Mahmood, as well as some other corner-case misbehaviors. The desired behavior is the same as we use in other type-unification cases: report the common typmod if there is one, but otherwise return -1 indicating no particular constraint. We fixed this in HEAD by deriving the typmods during transformValuesClause and storing them in the RTE, but that's not a feasible solution in the back branches. Instead, just use a brute-force approach of determining the correct common typmod during expandRTE() and get_rte_attribute_type(). Simple testing says that that doesn't really cost much, at least not in common cases where expandRTE() is only used once per query. It turns out that get_rte_attribute_type() is typically never used at all on VALUES RTEs, so the inefficiency there is of no great concern. Report: https://postgr.es/m/20161205143037.4377.60754@wrigleys.postgresql.org Discussion: https://postgr.es/m/27429.1480968538@sss.pgh.pa.us
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
@@ -50,6 +50,7 @@ static void expandTupleDesc(TupleDesc tupdesc, Alias *eref,
 				int rtindex, int sublevels_up,
 				int location, bool include_dropped,
 				List **colnames, List **colvars);
+static int32 *getValuesTypmods(RangeTblEntry *rte);
 static int	specialAttNum(const char *attname);
 static bool isQueryUsingTempRelation_walker(Node *node, void *context);
 
@@ -1814,9 +1815,22 @@ expandRTE(RangeTblEntry *rte, int rtindex, int sublevels_up,
 			{
 				/* Values RTE */
 				ListCell   *aliasp_item = list_head(rte->eref->colnames);
+				int32	   *coltypmods;
 				ListCell   *lcv;
 				ListCell   *lcc;
 
+				/*
+				 * It's okay to extract column types from the expressions in
+				 * the first row, since all rows will have been coerced to the
+				 * same types.  Their typmods might not be the same though, so
+				 * we potentially need to examine all rows to compute those.
+				 * Column collations are pre-computed in values_collations.
+				 */
+				if (colvars)
+					coltypmods = getValuesTypmods(rte);
+				else
+					coltypmods = NULL;
+
 				varattno = 0;
 				forboth(lcv, (List *) linitial(rte->values_lists),
 						lcc, rte->values_collations)
@@ -1841,13 +1855,15 @@ expandRTE(RangeTblEntry *rte, int rtindex, int sublevels_up,
 
 						varnode = makeVar(rtindex, varattno,
 										  exprType(col),
-										  exprTypmod(col),
+										  coltypmods[varattno - 1],
 										  colcollation,
 										  sublevels_up);
 						varnode->location = location;
 						*colvars = lappend(*colvars, varnode);
 					}
 				}
+				if (coltypmods)
+					pfree(coltypmods);
 			}
 			break;
 		case RTE_JOIN:
@@ -1953,6 +1969,8 @@ expandRTE(RangeTblEntry *rte, int rtindex, int sublevels_up,
 						varnode = makeVar(rtindex, varattno,
 										  coltype, coltypmod, colcoll,
 										  sublevels_up);
+						varnode->location = location;
+
 						*colvars = lappend(*colvars, varnode);
 					}
 				}
@@ -2043,6 +2061,74 @@ expandTupleDesc(TupleDesc tupdesc, Alias *eref,
 	}
 }
 
+/*
+ * getValuesTypmods -- expandRTE subroutine
+ *
+ * Identify per-column typmods for the given VALUES RTE.  Returns a
+ * palloc'd array.
+ */
+static int32 *
+getValuesTypmods(RangeTblEntry *rte)
+{
+	int32	   *coltypmods;
+	List	   *firstrow;
+	int			ncolumns,
+				nvalid,
+				i;
+	ListCell   *lc;
+
+	Assert(rte->values_lists != NIL);
+	firstrow = (List *) linitial(rte->values_lists);
+	ncolumns = list_length(firstrow);
+	coltypmods = (int32 *) palloc(ncolumns * sizeof(int32));
+	nvalid = 0;
+
+	/* Collect the typmods from the first VALUES row */
+	i = 0;
+	foreach(lc, firstrow)
+	{
+		Node	   *col = (Node *) lfirst(lc);
+
+		coltypmods[i] = exprTypmod(col);
+		if (coltypmods[i] >= 0)
+			nvalid++;
+		i++;
+	}
+
+	/*
+	 * Scan remaining rows; as soon as we have a non-matching typmod for a
+	 * column, reset that typmod to -1.  We can bail out early if all typmods
+	 * become -1.
+	 */
+	if (nvalid > 0)
+	{
+		for_each_cell(lc, lnext(list_head(rte->values_lists)))
+		{
+			List	   *thisrow = (List *) lfirst(lc);
+			ListCell   *lc2;
+
+			Assert(list_length(thisrow) == ncolumns);
+			i = 0;
+			foreach(lc2, thisrow)
+			{
+				Node	   *col = (Node *) lfirst(lc2);
+
+				if (coltypmods[i] >= 0 && coltypmods[i] != exprTypmod(col))
+				{
+					coltypmods[i] = -1;
+					nvalid--;
+				}
+				i++;
+			}
+
+			if (nvalid <= 0)
+				break;
+		}
+	}
+
+	return coltypmods;
+}
+
 /*
  * expandRelAttrs -
  *	  Workhorse for "*" expansion: produce a list of targetentries
@@ -2256,18 +2342,25 @@ get_rte_attribute_type(RangeTblEntry *rte, AttrNumber attnum,
 			break;
 		case RTE_VALUES:
 			{
-				/* Values RTE --- get type info from first sublist */
-				/* collation is stored separately, though */
+				/*
+				 * Values RTE --- we can get type info from first sublist, but
+				 * typmod may require scanning all sublists, and collation is
+				 * stored separately.  Using getValuesTypmods() is overkill,
+				 * but this path is taken so seldom for VALUES that it's not
+				 * worth writing extra code.
+				 */
 				List	   *collist = (List *) linitial(rte->values_lists);
 				Node	   *col;
+				int32	   *coltypmods = getValuesTypmods(rte);
 
 				if (attnum < 1 || attnum > list_length(collist))
 					elog(ERROR, "values list %s does not have attribute %d",
 						 rte->eref->aliasname, attnum);
 				col = (Node *) list_nth(collist, attnum - 1);
 				*vartype = exprType(col);
-				*vartypmod = exprTypmod(col);
+				*vartypmod = coltypmods[attnum - 1];
 				*varcollid = list_nth_oid(rte->values_collations, attnum - 1);
+				pfree(coltypmods);
 			}
 			break;
 		case RTE_JOIN:
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
@@ -288,6 +288,43 @@ SELECT relname, relkind, reloptions FROM pg_class
  mysecview4 | v       | {security_barrier=false}
 (4 rows)
 
+-- This test checks that proper typmods are assigned in a multi-row VALUES
+CREATE VIEW tt1 AS
+  SELECT * FROM (
+    VALUES
+       ('abc'::varchar(3), '0123456789', 42, 'abcd'::varchar(4)),
+       ('0123456789', 'abc'::varchar(3), 42.12, 'abc'::varchar(4))
+  ) vv(a,b,c,d);
+\d+ tt1
+                      View "testviewschm2.tt1"
+ Column |         Type         | Modifiers | Storage  | Description 
+--------+----------------------+-----------+----------+-------------
+ a      | character varying    |           | extended | 
+ b      | character varying    |           | extended | 
+ c      | numeric              |           | main     | 
+ d      | character varying(4) |           | extended | 
+View definition:
+ SELECT vv.a,
+    vv.b,
+    vv.c,
+    vv.d
+   FROM ( VALUES ('abc'::character varying(3),'0123456789'::character varying,42,'abcd'::character varying(4)), ('0123456789'::character varying,'abc'::character varying(3),42.12,'abc'::character varying(4))) vv(a, b, c, d);
+
+SELECT * FROM tt1;
+     a      |     b      |   c   |  d   
+------------+------------+-------+------
+ abc        | 0123456789 |    42 | abcd
+ 0123456789 | abc        | 42.12 | abc
+(2 rows)
+
+SELECT a::varchar(3) FROM tt1;
+  a  
+-----
+ abc
+ 012
+(2 rows)
+
+DROP VIEW tt1;
 -- Test view decompilation in the face of relation renaming conflicts
 CREATE TABLE tt1 (f1 int, f2 int, f3 text);
 CREATE TABLE tx1 (x1 int, x2 int, x3 text);
diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
@@ -224,6 +224,19 @@ SELECT relname, relkind, reloptions FROM pg_class
                      'mysecview3'::regclass, 'mysecview4'::regclass)
        ORDER BY relname;
 
+-- This test checks that proper typmods are assigned in a multi-row VALUES
+
+CREATE VIEW tt1 AS
+  SELECT * FROM (
+    VALUES
+       ('abc'::varchar(3), '0123456789', 42, 'abcd'::varchar(4)),
+       ('0123456789', 'abc'::varchar(3), 42.12, 'abc'::varchar(4))
+  ) vv(a,b,c,d);
+\d+ tt1
+SELECT * FROM tt1;
+SELECT a::varchar(3) FROM tt1;
+DROP VIEW tt1;
+
 -- Test view decompilation in the face of relation renaming conflicts
 
 CREATE TABLE tt1 (f1 int, f2 int, f3 text);
