From: Tom Lane <tgl@sss.pgh.pa.us>
To: Bruce Momjian <bruce@momjian.us>
cc: "David G. Johnston" <david.g.johnston@gmail.com>,
        Guyren Howe <guyren@gmail.com>,
        Pg Docs <pgsql-docs@lists.postgresql.org>
Subject: Re: Please provide examples of rows from
In-reply-to: <798909.1600562993@sss.pgh.pa.us>
References: <158638264419.662.2482095087061084020@wrigleys.postgresql.org>
 <20200904014618.GB13932@momjian.us>
 <CAKFQuwbBDr22KJRyJQRECO_XU6cyouwz3ocn0Tv4JgG4Gm5hMQ@mail.gmail.com>
 <20200920003910.GA22746@momjian.us> <798909.1600562993@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sat, 19 Sep 2020 20:49:53 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <877991.1600625321.0@sss.pgh.pa.us>
Date: Sun, 20 Sep 2020 14:09:13 -0400
Message-ID: <878016.1600625353@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <877991.1600625321.1@sss.pgh.pa.us>

I wrote:
> Yeah, this error message needs some help.  With a function having
> multiple OUT parameters, the prorettype is indeed "record", but
> the specific record type is implied by the OUT parameters so you
> do not need to (and can't) specify it in the query.
> The point of the AS feature is to allow specifying the concrete
> record type for record-returning functions that don't have a
> predefined result record type, like dblink().
> I think this error text was written before we had multiple OUT
> parameters, so it was okay at the time; but now it needs to be
> more precise.

Concretely, perhaps like the attached.  I was unsurprised to find
that this error condition isn't exercised in our regression tests.
I added some coverage, but maybe that's overkill?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="improve-message-for-unwanted-coldeflist.patch";
	charset="us-ascii"
Content-ID: <877991.1600625321.2@sss.pgh.pa.us>
Content-Description: improve-message-for-unwanted-coldeflist.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/pars=
e_relation.c
index b875a50646..a56bd86181 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1737,16 +1737,46 @@ addRangeTableEntryForFunction(ParseState *pstate,
 =

 		/*
 		 * A coldeflist is required if the function returns RECORD and hasn't
-		 * got a predetermined record type, and is prohibited otherwise.
+		 * got a predetermined record type, and is prohibited otherwise.  This
+		 * can be a bit confusing, so we expend some effort on delivering a
+		 * relevant error message.
 		 */
 		if (coldeflist !=3D NIL)
 		{
-			if (functypclass !=3D TYPEFUNC_RECORD)
-				ereport(ERROR,
-						(errcode(ERRCODE_SYNTAX_ERROR),
-						 errmsg("a column definition list is only allowed for functions ret=
urning \"record\""),
-						 parser_errposition(pstate,
-											exprLocation((Node *) coldeflist))));
+			switch (functypclass)
+			{
+				case TYPEFUNC_RECORD:
+					/* ok */
+					break;
+				case TYPEFUNC_COMPOSITE:
+				case TYPEFUNC_COMPOSITE_DOMAIN:
+
+					/*
+					 * If the function's raw result type is RECORD, we must
+					 * have resolved it using its OUT parameters.  Otherwise,
+					 * it must have a named composite type.
+					 */
+					if (exprType(funcexpr) =3D=3D RECORDOID)
+						ereport(ERROR,
+								(errcode(ERRCODE_SYNTAX_ERROR),
+								 errmsg("a column definition list is redundant for a function wit=
h OUT parameters"),
+								 parser_errposition(pstate,
+													exprLocation((Node *) coldeflist))));
+					else
+						ereport(ERROR,
+								(errcode(ERRCODE_SYNTAX_ERROR),
+								 errmsg("a column definition list is redundant for a function ret=
urning a named composite type"),
+								 parser_errposition(pstate,
+													exprLocation((Node *) coldeflist))));
+					break;
+				default:
+					ereport(ERROR,
+							(errcode(ERRCODE_SYNTAX_ERROR),
+							 errmsg("a column definition list is only allowed for functions re=
turning \"record\""),
+							 parser_errposition(pstate,
+												exprLocation((Node *) coldeflist))));
+					break;
+			}
 		}
 		else
 		{
diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/e=
xpected/rangefuncs.out
index 7eced28452..e618aec2eb 100644
--- a/src/test/regress/expected/rangefuncs.out
+++ b/src/test/regress/expected/rangefuncs.out
@@ -2109,6 +2109,19 @@ select * from testrngfunc();
  7.136178 | 7.14
 (1 row)
 =

+-- Check a couple of error cases while we're here
+select * from testrngfunc() as t(f1 int8,f2 int8);  -- fail, composite re=
sult
+ERROR:  a column definition list is redundant for a function returning a =
named composite type
+LINE 1: select * from testrngfunc() as t(f1 int8,f2 int8);
+                                         ^
+select * from pg_get_keywords() as t(f1 int8,f2 int8);  -- fail, OUT para=
ms
+ERROR:  a column definition list is redundant for a function with OUT par=
ameters
+LINE 1: select * from pg_get_keywords() as t(f1 int8,f2 int8);
+                                             ^
+select * from sin(3) as t(f1 int8,f2 int8);  -- fail, scalar result type
+ERROR:  a column definition list is only allowed for functions returning =
"record"
+LINE 1: select * from sin(3) as t(f1 int8,f2 int8);
+                                  ^
 drop type rngfunc_type cascade;
 NOTICE:  drop cascades to function testrngfunc()
 --
diff --git a/src/test/regress/sql/rangefuncs.sql b/src/test/regress/sql/ra=
ngefuncs.sql
index ae3119a959..5f41cb2d8d 100644
--- a/src/test/regress/sql/rangefuncs.sql
+++ b/src/test/regress/sql/rangefuncs.sql
@@ -629,6 +629,11 @@ explain (verbose, costs off)
 select * from testrngfunc();
 select * from testrngfunc();
 =

+-- Check a couple of error cases while we're here
+select * from testrngfunc() as t(f1 int8,f2 int8);  -- fail, composite re=
sult
+select * from pg_get_keywords() as t(f1 int8,f2 int8);  -- fail, OUT para=
ms
+select * from sin(3) as t(f1 int8,f2 int8);  -- fail, scalar result type
+
 drop type rngfunc_type cascade;
 =

 --

------- =_aaaaaaaaaa0--