From: Tom Lane Date: Fri, 9 Dec 2016 17:01:14 +0000 (-0500) Subject: Fix reporting of column typmods for multi-row VALUES constructs. X-Git-Tag: REL9_6_2~89 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=cf22c8cb8900e09615e3791590507f1edef8dd97;p=postgresql Fix reporting of column typmods for multi-row VALUES constructs. 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 index 1e3ecbc51e..c51fd81b63 100644 --- a/src/backend/parser/parse_relation.c +++ b/src/backend/parser/parse_relation.c @@ -52,6 +52,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); @@ -2157,9 +2158,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) @@ -2184,13 +2198,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: @@ -2296,6 +2312,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); } } @@ -2412,6 +2430,74 @@ expandTupleDesc(TupleDesc tupdesc, Alias *eref, int count, int offset, } } +/* + * 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 @@ -2656,18 +2742,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 index 81b4e8d42b..b1c3cff931 100644 --- 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 index 8bed5a53b3..5fe8b94aae 100644 --- 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);