]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Revert applying column aliases to the output of whole-row Vars.
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 17 Mar 2022 22:18:05 +0000 (18:18 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 17 Mar 2022 22:18:05 +0000 (18:18 -0400)
In commit bf7ca1587, I had the bright idea that we could make the
result of a whole-row Var (that is, foo.*) track any column aliases
that had been applied to the FROM entry the Var refers to.  However,
that's not terribly logically consistent, because now the output of
the Var is no longer of the named composite type that the Var claims
to emit.  bf7ca1587 tried to handle that by changing the output
tuple values to be labeled with a blessed RECORD type, but that's
really pretty disastrous: we can wind up storing such tuples onto
disk, whereupon they're not readable by other sessions.

The only practical fix I can see is to give up on what bf7ca1587
tried to do, and say that the column names of tuples produced by
a whole-row Var are always those of the underlying named composite
type, query aliases or no.  While this introduces some inconsistencies,
it removes others, so it's not that awful in the abstract.  What *is*
kind of awful is to make such a behavioral change in a back-patched
bug fix.  But corrupt data is worse, so back-patched it will be.

(A workaround available to anyone who's unhappy about this is to
introduce an extra level of sub-SELECT, so that the whole-row Var is
referring to the sub-SELECT's output and not to a named table type.
Then the Var is of type RECORD to begin with and there's no issue.)

Per report from Miles Delahunty.  The faulty commit dates to 9.5,
so back-patch to all supported branches.

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

src/backend/executor/execExpr.c
src/backend/executor/execExprInterp.c
src/backend/executor/execTuples.c
src/test/regress/expected/rowtypes.out
src/test/regress/sql/rowtypes.sql

index 2a2741352af88094d2ee6abe8a0a4d31b9abded3..08ae58a68165e93c3d7fd16c4839de5d24bcf517 100644 (file)
@@ -1532,16 +1532,16 @@ ExecInitExprRec(Expr *node, PlanState *parent, ExprState *state,
                                {
                                        /* generic record, use types of given expressions */
                                        tupdesc = ExecTypeFromExprList(rowexpr->args);
+                                       /* ... but adopt RowExpr's column aliases */
+                                       ExecTypeSetColNames(tupdesc, rowexpr->colnames);
+                                       /* Bless the tupdesc so it can be looked up later */
+                                       BlessTupleDesc(tupdesc);
                                }
                                else
                                {
                                        /* it's been cast to a named type, use that */
                                        tupdesc = lookup_rowtype_tupdesc_copy(rowexpr->row_typeid, -1);
                                }
-                               /* In either case, adopt RowExpr's column aliases */
-                               ExecTypeSetColNames(tupdesc, rowexpr->colnames);
-                               /* Bless the tupdesc in case it's now of type RECORD */
-                               BlessTupleDesc(tupdesc);
 
                                /*
                                 * In the named-type case, the tupdesc could have more columns
index de51561769506fd62da97b24b007932afbd66f74..7a20c11bdb481b87a936c390c2a62f24809a9b7e 100644 (file)
@@ -3475,9 +3475,8 @@ ExecEvalWholeRowVar(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
 
                        /*
                         * Use the variable's declared rowtype as the descriptor for the
-                        * output values, modulo possibly assigning new column names
-                        * below. In particular, we *must* absorb any attisdropped
-                        * markings.
+                        * output values.  In particular, we *must* absorb any
+                        * attisdropped markings.
                         */
                        oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
                        output_tupdesc = CreateTupleDescCopy(var_tupdesc);
@@ -3495,39 +3494,38 @@ ExecEvalWholeRowVar(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
                        oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
                        output_tupdesc = CreateTupleDescCopy(slot->tts_tupleDescriptor);
                        MemoryContextSwitchTo(oldcontext);
-               }
 
-               /*
-                * Construct a tuple descriptor for the composite values we'll
-                * produce, and make sure its record type is "blessed".  The main
-                * reason to do this is to be sure that operations such as
-                * row_to_json() will see the desired column names when they look up
-                * the descriptor from the type information embedded in the composite
-                * values.
-                *
-                * We already got the correct physical datatype info above, but now we
-                * should try to find the source RTE and adopt its column aliases, in
-                * case they are different from the original rowtype's names.  For
-                * example, in "SELECT foo(t) FROM tab t(x,y)", the first two columns
-                * in the composite output should be named "x" and "y" regardless of
-                * tab's column names.
-                *
-                * If we can't locate the RTE, assume the column names we've got are
-                * OK.  (As of this writing, the only cases where we can't locate the
-                * RTE are in execution of trigger WHEN clauses, and then the Var will
-                * have the trigger's relation's rowtype, so its names are fine.)
-                * Also, if the creator of the RTE didn't bother to fill in an eref
-                * field, assume our column names are OK.  (This happens in COPY, and
-                * perhaps other places.)
-                */
-               if (econtext->ecxt_estate &&
-                       variable->varno <= list_length(econtext->ecxt_estate->es_range_table))
-               {
-                       RangeTblEntry *rte = rt_fetch(variable->varno,
-                                                                                 econtext->ecxt_estate->es_range_table);
+                       /*
+                        * It's possible that the input slot is a relation scan slot and
+                        * so is marked with that relation's rowtype.  But we're supposed
+                        * to be returning RECORD, so reset to that.
+                        */
+                       output_tupdesc->tdtypeid = RECORDOID;
+                       output_tupdesc->tdtypmod = -1;
+
+                       /*
+                        * We already got the correct physical datatype info above, but
+                        * now we should try to find the source RTE and adopt its column
+                        * aliases, since it's unlikely that the input slot has the
+                        * desired names.
+                        *
+                        * If we can't locate the RTE, assume the column names we've got
+                        * are OK.  (As of this writing, the only cases where we can't
+                        * locate the RTE are in execution of trigger WHEN clauses, and
+                        * then the Var will have the trigger's relation's rowtype, so its
+                        * names are fine.)  Also, if the creator of the RTE didn't bother
+                        * to fill in an eref field, assume our column names are OK. (This
+                        * happens in COPY, and perhaps other places.)
+                        */
+                       if (econtext->ecxt_estate &&
+                               variable->varno <= list_length(econtext->ecxt_estate->es_range_table))
+                       {
+                               RangeTblEntry *rte = rt_fetch(variable->varno,
+                                                                                         econtext->ecxt_estate->es_range_table);
 
-                       if (rte->eref)
-                               ExecTypeSetColNames(output_tupdesc, rte->eref->colnames);
+                               if (rte->eref)
+                                       ExecTypeSetColNames(output_tupdesc, rte->eref->colnames);
+                       }
                }
 
                /* Bless the tupdesc if needed, and save it in the execution state */
index 7ae70a877a0c306785154c1403b9b8fd3c32ae3f..e64868feb3f4d3dca3c978a608be66cd1d91f0e7 100644 (file)
@@ -972,50 +972,39 @@ ExecTypeFromExprList(List *exprList)
 }
 
 /*
- * ExecTypeSetColNames - set column names in a TupleDesc
+ * ExecTypeSetColNames - set column names in a RECORD TupleDesc
  *
  * Column names must be provided as an alias list (list of String nodes).
- *
- * For some callers, the supplied tupdesc has a named rowtype (not RECORD)
- * and it is moderately likely that the alias list matches the column names
- * already present in the tupdesc.  If we do change any column names then
- * we must reset the tupdesc's type to anonymous RECORD; but we avoid doing
- * so if no names change.
  */
 void
 ExecTypeSetColNames(TupleDesc typeInfo, List *namesList)
 {
-       bool            modified = false;
        int                     colno = 0;
        ListCell   *lc;
 
+       /* It's only OK to change col names in a not-yet-blessed RECORD type */
+       Assert(typeInfo->tdtypeid == RECORDOID);
+       Assert(typeInfo->tdtypmod < 0);
+
        foreach(lc, namesList)
        {
                char       *cname = strVal(lfirst(lc));
                Form_pg_attribute attr;
 
-               /* Guard against too-long names list */
+               /* Guard against too-long names list (probably can't happen) */
                if (colno >= typeInfo->natts)
                        break;
                attr = typeInfo->attrs[colno++];
 
-               /* Ignore empty aliases (these must be for dropped columns) */
-               if (cname[0] == '\0')
+               /*
+                * Do nothing for empty aliases or dropped columns (these cases
+                * probably can't arise in RECORD types, either)
+                */
+               if (cname[0] == '\0' || attr->attisdropped)
                        continue;
 
-               /* Change tupdesc only if alias is actually different */
-               if (strcmp(cname, NameStr(attr->attname)) != 0)
-               {
-                       namestrcpy(&(attr->attname), cname);
-                       modified = true;
-               }
-       }
-
-       /* If we modified the tupdesc, it's now a new record type */
-       if (modified)
-       {
-               typeInfo->tdtypeid = RECORDOID;
-               typeInfo->tdtypmod = -1;
+               /* OK, assign the column name */
+               namestrcpy(&(attr->attname), cname);
        }
 }
 
index 43b36f6566d3cd4634d8b1685b32c53879ccc58e..b1998b9a1d9746804207fb88d007c9323e9af2b5 100644 (file)
@@ -511,18 +511,8 @@ select row_to_json(i) from int8_tbl i;
  {"q1":4567890123456789,"q2":-4567890123456789}
 (5 rows)
 
+-- since "i" is of type "int8_tbl", attaching aliases doesn't change anything:
 select row_to_json(i) from int8_tbl i(x,y);
-                 row_to_json                  
-----------------------------------------------
- {"x":123,"y":456}
- {"x":123,"y":4567890123456789}
- {"x":4567890123456789,"y":123}
- {"x":4567890123456789,"y":4567890123456789}
- {"x":4567890123456789,"y":-4567890123456789}
-(5 rows)
-
-create temp view vv1 as select * from int8_tbl;
-select row_to_json(i) from vv1 i;
                   row_to_json                   
 ------------------------------------------------
  {"q1":123,"q2":456}
@@ -532,16 +522,7 @@ select row_to_json(i) from vv1 i;
  {"q1":4567890123456789,"q2":-4567890123456789}
 (5 rows)
 
-select row_to_json(i) from vv1 i(x,y);
-                 row_to_json                  
-----------------------------------------------
- {"x":123,"y":456}
- {"x":123,"y":4567890123456789}
- {"x":4567890123456789,"y":123}
- {"x":4567890123456789,"y":4567890123456789}
- {"x":4567890123456789,"y":-4567890123456789}
-(5 rows)
-
+-- in these examples, we'll report the exposed column names of the subselect:
 select row_to_json(ss) from
   (select q1, q2 from int8_tbl) as ss;
                   row_to_json                   
index 8d63060500a0d80ac9c4fd8c35abcb5cacf5b9ea..6537afe62355c701d5eba2c253697afee9618773 100644 (file)
@@ -249,12 +249,10 @@ select (row('Jim', 'Beam')).text;  -- error
 --
 
 select row_to_json(i) from int8_tbl i;
+-- since "i" is of type "int8_tbl", attaching aliases doesn't change anything:
 select row_to_json(i) from int8_tbl i(x,y);
 
-create temp view vv1 as select * from int8_tbl;
-select row_to_json(i) from vv1 i;
-select row_to_json(i) from vv1 i(x,y);
-
+-- in these examples, we'll report the exposed column names of the subselect:
 select row_to_json(ss) from
   (select q1, q2 from int8_tbl) as ss;
 select row_to_json(ss) from