Revert applying column aliases to the output of whole-row Vars.

tglsfdc · tglsfdc · commit 1f5ef5ae0806 · 2022-03-17T18:18:05.000-04:00
In commit bf7ca15, 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. bf7ca15 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 bf7ca15 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
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
@@ -1629,16 +1629,16 @@ ExecInitExprRec(Expr *node, 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
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
@@ -3917,12 +3917,8 @@ ExecEvalWholeRowVar(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
 			 * generates an INT4 NULL regardless of the dropped column type).
 			 * If we find a dropped column and cannot verify that case (1)
 			 * holds, we have to use the slow path to check (2) for each row.
-			 *
-			 * If vartype is a domain over composite, just look through that
-			 * to the base composite type.
 			 */
-			var_tupdesc = lookup_rowtype_tupdesc_domain(variable->vartype,
-														-1, false);
+			var_tupdesc = lookup_rowtype_tupdesc(variable->vartype, -1);
 
 			slot_tupdesc = slot->tts_tupleDescriptor;
 
@@ -3959,9 +3955,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);
@@ -3979,39 +3974,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 <= econtext->ecxt_estate->es_range_table_size)
-		{
-			RangeTblEntry *rte = exec_rt_fetch(variable->varno,
-											   econtext->ecxt_estate);
+			/*
+			 * 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;
 
-			if (rte->eref)
-				ExecTypeSetColNames(output_tupdesc, rte->eref->colnames);
+			/*
+			 * 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 <= econtext->ecxt_estate->es_range_table_size)
+			{
+				RangeTblEntry *rte = exec_rt_fetch(variable->varno,
+												   econtext->ecxt_estate);
+
+				if (rte->eref)
+					ExecTypeSetColNames(output_tupdesc, rte->eref->colnames);
+			}
 		}
 
 		/* Bless the tupdesc if needed, and save it in the execution state */
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
@@ -2022,51 +2022,40 @@ 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 = TupleDescAttr(typeInfo, colno);
 		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);
 	}
 }
 
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
@@ -965,18 +965,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}
@@ -986,16 +976,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                   
diff --git a/src/test/regress/sql/rowtypes.sql b/src/test/regress/sql/rowtypes.sql
@@ -404,12 +404,10 @@ select longname(f) from fullname f;
 --
 
 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

Original file line number	Diff line number	Diff line change
`@@ -1629,16 +1629,16 @@ ExecInitExprRec(Expr node, ExprState state,`
`1629`	`1629`	`{`
`1630`	`1630`	`/* generic record, use types of given expressions */`
`1631`	`1631`	`tupdesc = ExecTypeFromExprList(rowexpr->args);`
	`1632`	`+ /* ... but adopt RowExpr's column aliases */`
	`1633`	`+ ExecTypeSetColNames(tupdesc, rowexpr->colnames);`
	`1634`	`+ /* Bless the tupdesc so it can be looked up later */`
	`1635`	`+ BlessTupleDesc(tupdesc);`
`1632`	`1636`	`}`
`1633`	`1637`	`else`
`1634`	`1638`	`{`
`1635`	`1639`	`/* it's been cast to a named type, use that */`
`1636`	`1640`	`tupdesc = lookup_rowtype_tupdesc_copy(rowexpr->row_typeid, -1);`
`1637`	`1641`	`}`
`1638`		`- /* In either case, adopt RowExpr's column aliases */`
`1639`		`- ExecTypeSetColNames(tupdesc, rowexpr->colnames);`
`1640`		`- /* Bless the tupdesc in case it's now of type RECORD */`
`1641`		`- BlessTupleDesc(tupdesc);`
`1642`	`1642`
`1643`	`1643`	`/*`
`1644`	`1644`	`* In the named-type case, the tupdesc could have more columns`