Sender: Elvis Pranskevichus <elprans@gmail.com>
From: Elvis Pranskevichus <el@prans.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH] Allow anonymous rowtypes in function return column
 definition
Date: Mon, 28 Jan 2019 12:52:57 -0500
Message-ID: <1691096.MngoxCRQ0q@hammer.magicstack.net>
In-Reply-To: <16189.1547416668@sss.pgh.pa.us>
References: <11038447.kQ5A9Uj5xi@hammer.magicstack.net>
 <16189.1547416668@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nextPart2266730.DszM2FW0kA"
Content-Transfer-Encoding: 7Bit
Precedence: bulk

This is a multi-part message in MIME format.

--nextPart2266730.DszM2FW0kA
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"

On Sunday, January 13, 2019 4:57:48 PM EST Tom Lane wrote:

> I also wonder why we'd allow RECORD but not RECORDARRAY.  

That's by omission.  There's no reason to refuse RECORDARRAY here for 
the same reason why RECORD is accepted.

> More generally, why not any polymorphic type? [...] the
> fact that a record value is self-identifying as long as you know
> it's a record, whereas a random Datum is not a self-identifying
> member of the type class "anyelement", for instance.

Yes that's the reason.  We allow "record" in coldeflist because it 
only happens near a RangeFunction, and set-returning functions always do 
"BlessTupleDesc", which means that RECORDOID data would always have an 
associated TupleDesc and can be processed as a regular composite type.  
Recursion is not an issue, since every instance would have a separate 
TupleDesc even if the shape is the same.

> I feel a bit uncomfortable about defining the new flag to
> CheckAttributeNamesTypes as "allow_anonymous_records"; that
> seems pretty short-sighted and single-purpose, especially in
> view of there being no obvious reason why it shouldn't accept
> RECORDARRAY too.  Maybe with a clearer explanation of the
> issues above, we could define that flag in a more on-point way.

I renamed it to "in_coldeflist", which seems like a clearer indication 
that we are validating that, and not a regular table definition.

> BTW, it strikes me that maybe the reason ANYARRAY isn't insane
> to allow in pg_statistic has to do with arrays also being
> self-identifying members of that type class

That's true.  Array data encode the OID of their element type, but that 
only allows sending the data out, as subscripting or casting anyarray is 
not allowed.  There also seems to be no guarantee that the actual type 
of the array doesn't change from row to row in such a scenario.  Given 
that, I think it would be best to keep anyarray restricted to the system 
catalogs.

I attached the updated patch.
                             
                               Elvis
--nextPart2266730.DszM2FW0kA
Content-Disposition: attachment;
 filename="Allow-anonymous-rowtypes-in-coldeflist-v2.patch"
Content-Transfer-Encoding: 7Bit
Content-Type: text/x-patch; charset="UTF-8";
 name="Allow-anonymous-rowtypes-in-coldeflist-v2.patch"

From 2841fd41db106ae09b68d3b5cf29901e98e2446a Mon Sep 17 00:00:00 2001
From: Elvis Pranskevichus <elvis@magic.io>
Date: Thu, 6 Dec 2018 17:16:28 -0500
Subject: [PATCH] Allow anonymous rowtypes in function return column definition

Currently, the following query

    SELECT q.b = row(2)
    FROM unnest(ARRAY[row(1, row(2))]) AS q(a int, b record);

would fail with

    ERROR:  column "b" has pseudo-type record

This is due to CheckAttributeNamesTypes() being used on a function
coldeflist as if it was a real relation definition.  But in the context
of a query there seems to be no harm in allowing this, as other ways of
manipulating anonymous rowtypes work well, e.g.:

    SELECT (ARRAY[ROW(1, ROW(2))])[1];
---
 src/backend/catalog/heap.c             | 42 ++++++++++++++++++--------
 src/backend/catalog/index.c            |  2 +-
 src/backend/commands/tablecmds.c       |  4 +--
 src/backend/parser/parse_relation.c    |  5 +--
 src/include/catalog/heap.h             |  6 ++--
 src/test/regress/expected/rowtypes.out | 10 ++++++
 src/test/regress/sql/rowtypes.sql      |  5 +++
 7 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index cc865de627..103b4fb96a 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -451,7 +451,8 @@ heap_create(const char *relname,
  */
 void
 CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind,
-						 bool allow_system_table_mods)
+						 bool allow_system_table_mods,
+						 bool in_coldeflist)
 {
 	int			i;
 	int			j;
@@ -509,7 +510,8 @@ CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind,
 						   TupleDescAttr(tupdesc, i)->atttypid,
 						   TupleDescAttr(tupdesc, i)->attcollation,
 						   NIL, /* assume we're creating a new rowtype */
-						   allow_system_table_mods);
+						   allow_system_table_mods,
+						   in_coldeflist);
 	}
 }
 
@@ -532,7 +534,8 @@ void
 CheckAttributeType(const char *attname,
 				   Oid atttypid, Oid attcollation,
 				   List *containing_rowtypes,
-				   bool allow_system_table_mods)
+				   bool allow_system_table_mods,
+				   bool in_coldeflist)
 {
 	char		att_typtype = get_typtype(atttypid);
 	Oid			att_typelem;
@@ -540,12 +543,24 @@ CheckAttributeType(const char *attname,
 	if (att_typtype == TYPTYPE_PSEUDO)
 	{
 		/*
-		 * Refuse any attempt to create a pseudo-type column, except for a
-		 * special hack for pg_statistic: allow ANYARRAY when modifying system
-		 * catalogs (this allows creating pg_statistic and cloning it during
-		 * VACUUM FULL)
+		 * Generally speaking, we don't allow pseudo-types in column
+		 * definitions, with the following exceptions:
+		 *
+		 * First, we allow the record pseudo-type (and its array type) in
+		 * the column definition of a set-returning function.  In this case
+		 * the record type is "blessed", i.e. there is an actual TupleDesc
+		 * associated with it and it can be treated as a normal composite
+		 * type.  Composite type recursion is not a danger here since
+		 * each instance of an anonymous rowtype in this context would have
+		 * a distinct TupleDesc.
+		 *
+		 * Secondly, we allow allow ANYARRAY when modifying system
+		 * catalogs (this allows creating pg_statistic and cloning
+		 * it during VACUUM FULL).
 		 */
-		if (atttypid != ANYARRAYOID || !allow_system_table_mods)
+		if (!((atttypid == ANYARRAYOID && allow_system_table_mods) ||
+				((atttypid == RECORDOID || atttypid == RECORDARRAYOID) &&
+					in_coldeflist)))
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 					 errmsg("column \"%s\" has pseudo-type %s",
@@ -558,7 +573,8 @@ CheckAttributeType(const char *attname,
 		 */
 		CheckAttributeType(attname, getBaseType(atttypid), attcollation,
 						   containing_rowtypes,
-						   allow_system_table_mods);
+						   allow_system_table_mods,
+						   in_coldeflist);
 	}
 	else if (att_typtype == TYPTYPE_COMPOSITE)
 	{
@@ -596,7 +612,8 @@ CheckAttributeType(const char *attname,
 			CheckAttributeType(NameStr(attr->attname),
 							   attr->atttypid, attr->attcollation,
 							   containing_rowtypes,
-							   allow_system_table_mods);
+							   allow_system_table_mods,
+							   in_coldeflist);
 		}
 
 		relation_close(relation, AccessShareLock);
@@ -610,7 +627,8 @@ CheckAttributeType(const char *attname,
 		 */
 		CheckAttributeType(attname, att_typelem, attcollation,
 						   containing_rowtypes,
-						   allow_system_table_mods);
+						   allow_system_table_mods,
+						   in_coldeflist);
 	}
 
 	/*
@@ -1076,7 +1094,7 @@ heap_create_with_catalog(const char *relname,
 	 */
 	Assert(IsNormalProcessingMode() || IsBootstrapProcessingMode());
 
-	CheckAttributeNamesTypes(tupdesc, relkind, allow_system_table_mods);
+	CheckAttributeNamesTypes(tupdesc, relkind, allow_system_table_mods, false);
 
 	/*
 	 * This would fail later on anyway, if the relation already exists.  But
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 225c078018..9a7f7aabbf 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -411,7 +411,7 @@ ConstructTupleDescriptor(Relation heapRelation,
 			 */
 			CheckAttributeType(NameStr(to->attname),
 							   to->atttypid, to->attcollation,
-							   NIL, false);
+							   NIL, false, false);
 		}
 
 		/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ff76499137..8383b419aa 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5509,7 +5509,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	/* make sure datatype is legal for a column */
 	CheckAttributeType(colDef->colname, typeOid, collOid,
 					   list_make1_oid(rel->rd_rel->reltype),
-					   false);
+					   false, false);
 
 	/* construct new attribute's pg_attribute entry */
 	attribute.attrelid = myrelid;
@@ -9449,7 +9449,7 @@ ATPrepAlterColumnType(List **wqueue,
 	/* make sure datatype is legal for a column */
 	CheckAttributeType(colName, targettype, targetcollid,
 					   list_make1_oid(rel->rd_rel->reltype),
-					   false);
+					   false, false);
 
 	if (tab->relkind == RELKIND_RELATION ||
 		tab->relkind == RELKIND_PARTITIONED_TABLE)
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 09fbb588af..2106d5bcd7 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1590,9 +1590,10 @@ addRangeTableEntryForFunction(ParseState *pstate,
 
 			/*
 			 * Ensure that the coldeflist defines a legal set of names (no
-			 * duplicates) and datatypes (no pseudo-types, for instance).
+			 * duplicates) and datatypes (no pseudo-types, for instance, but
+			 * record, and record[] are allowed).
 			 */
-			CheckAttributeNamesTypes(tupdesc, RELKIND_COMPOSITE_TYPE, false);
+			CheckAttributeNamesTypes(tupdesc, RELKIND_COMPOSITE_TYPE, false, true);
 		}
 		else
 			ereport(ERROR,
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 625b7e5c43..962ca08eab 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -130,12 +130,14 @@ extern const FormData_pg_attribute *SystemAttributeDefinition(AttrNumber attno);
 extern const FormData_pg_attribute *SystemAttributeByName(const char *attname);
 
 extern void CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind,
-						 bool allow_system_table_mods);
+						 bool allow_system_table_mods,
+						 bool allow_anonymous_records);
 
 extern void CheckAttributeType(const char *attname,
 				   Oid atttypid, Oid attcollation,
 				   List *containing_rowtypes,
-				   bool allow_system_table_mods);
+				   bool allow_system_table_mods,
+				   bool allow_anonymous_records);
 
 /* pg_partitioned_table catalog manipulation functions */
 extern void StorePartitionKey(Relation rel,
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index d6a1a3331e..aaeffbc2a2 100644
--- a/src/test/regress/expected/rowtypes.out
+++ b/src/test/regress/expected/rowtypes.out
@@ -668,6 +668,16 @@ select row(1, '(1,2)')::testtype6 *<> row(1, '(1,3)')::testtype6;
 (1 row)
 
 drop type testtype1, testtype2, testtype3, testtype4, testtype5, testtype6;
+-- Test anonymous rowtype in coldeflist
+SELECT q.b = row(2), q.c = ARRAY[row(3)], q.d = row(row(4)) FROM
+    unnest(ARRAY[
+      row(1, row(2), ARRAY[row(3)], row(row(4)))
+    ]) AS q(a int, b record, c record[], d record);
+ ?column? | ?column? | ?column? 
+----------+----------+----------
+ t        | t        | t
+(1 row)
+
 --
 -- Test case derived from bug #5716: check multiple uses of a rowtype result
 --
diff --git a/src/test/regress/sql/rowtypes.sql b/src/test/regress/sql/rowtypes.sql
index e6d389805c..5fe7a48788 100644
--- a/src/test/regress/sql/rowtypes.sql
+++ b/src/test/regress/sql/rowtypes.sql
@@ -261,6 +261,11 @@ select row(1, '(1,2)')::testtype6 *<> row(1, '(1,3)')::testtype6;
 
 drop type testtype1, testtype2, testtype3, testtype4, testtype5, testtype6;
 
+-- Test anonymous rowtype in coldeflist
+SELECT q.b = row(2), q.c = ARRAY[row(3)], q.d = row(row(4)) FROM
+    unnest(ARRAY[
+      row(1, row(2), ARRAY[row(3)], row(row(4)))
+    ]) AS q(a int, b record, c record[], d record);
 
 --
 -- Test case derived from bug #5716: check multiple uses of a rowtype result
-- 
2.19.2


--nextPart2266730.DszM2FW0kA--