Skip to content

Commit e6c178b

Browse files
tglsfdctglsfdc
committed
Refactor our checks for valid function and aggregate signatures.
pg_proc.c and pg_aggregate.c had near-duplicate copies of the logic to decide whether a function or aggregate's signature is legal. This seems like a bad thing even without the problem that the upcoming "anycompatible" patch would roughly double the complexity of that logic. Hence, refactor so that the rules are localized in new subroutines supplied by parse_coerce.c. (One could quibble about just where to add that code, but putting it beside enforce_generic_type_consistency seems not totally unreasonable.) The fact that the rules are about to change would mandate some changes in the wording of the associated error messages in any case. I ended up spelling things out in a fairly literal fashion in the errdetail messages, eg "A result of type anyelement requires at least one input of type anyelement, anyarray, anynonarray, anyenum, or anyrange." Perhaps this is overkill, but once there's more than one subgroup of polymorphic types, people might get confused by more-abstract messages. Discussion: https://postgr.es/m/24137.1584139352@sss.pgh.pa.us
1 parent dbbb553 commit e6c178b

File tree

8 files changed

+176
-122
lines changed

8 files changed

+176
-122
lines changed

‎src/backend/catalog/pg_aggregate.c

Lines changed: 27 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,6 @@ AggregateCreate(const char *aggName,
9393
Oid mfinalfn = InvalidOid; /* can be omitted */
9494
Oid sortop = InvalidOid; /* can be omitted */
9595
Oid *aggArgTypes = parameterTypes->values;
96-
bool hasPolyArg;
97-
bool hasInternalArg;
9896
bool mtransIsStrict = false;
9997
Oid rettype;
10098
Oid finaltype;
@@ -103,6 +101,7 @@ AggregateCreate(const char *aggName,
103101
int nargs_finalfn;
104102
Oid procOid;
105103
TupleDesc tupDesc;
104+
char *detailmsg;
106105
int i;
107106
ObjectAddress myself,
108107
referenced;
@@ -131,36 +130,33 @@ AggregateCreate(const char *aggName,
131130
FUNC_MAX_ARGS - 1,
132131
FUNC_MAX_ARGS - 1)));
133132

134-
/* check for polymorphic and INTERNAL arguments */
135-
hasPolyArg = false;
136-
hasInternalArg = false;
137-
for (i = 0; i < numArgs; i++)
138-
{
139-
if (IsPolymorphicType(aggArgTypes[i]))
140-
hasPolyArg = true;
141-
else if (aggArgTypes[i] == INTERNALOID)
142-
hasInternalArg = true;
143-
}
144-
145133
/*
146134
* If transtype is polymorphic, must have polymorphic argument also; else
147135
* we will have no way to deduce the actual transtype.
148136
*/
149-
if (IsPolymorphicType(aggTransType) && !hasPolyArg)
137+
detailmsg = check_valid_polymorphic_signature(aggTransType,
138+
aggArgTypes,
139+
numArgs);
140+
if (detailmsg)
150141
ereport(ERROR,
151142
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
152143
errmsg("cannot determine transition data type"),
153-
errdetail("An aggregate using a polymorphic transition type must have at least one polymorphic argument.")));
144+
errdetail_internal("%s", detailmsg)));
154145

155146
/*
156147
* Likewise for moving-aggregate transtype, if any
157148
*/
158-
if (OidIsValid(aggmTransType) &&
159-
IsPolymorphicType(aggmTransType) && !hasPolyArg)
160-
ereport(ERROR,
161-
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
162-
errmsg("cannot determine transition data type"),
163-
errdetail("An aggregate using a polymorphic transition type must have at least one polymorphic argument.")));
149+
if (OidIsValid(aggmTransType))
150+
{
151+
detailmsg = check_valid_polymorphic_signature(aggmTransType,
152+
aggArgTypes,
153+
numArgs);
154+
if (detailmsg)
155+
ereport(ERROR,
156+
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
157+
errmsg("cannot determine transition data type"),
158+
errdetail_internal("%s", detailmsg)));
159+
}
164160

165161
/*
166162
* An ordered-set aggregate that is VARIADIC must be VARIADIC ANY. In
@@ -492,24 +488,29 @@ AggregateCreate(const char *aggName,
492488
* that itself violates the rule against polymorphic result with no
493489
* polymorphic input.)
494490
*/
495-
if (IsPolymorphicType(finaltype) && !hasPolyArg)
491+
detailmsg = check_valid_polymorphic_signature(finaltype,
492+
aggArgTypes,
493+
numArgs);
494+
if (detailmsg)
496495
ereport(ERROR,
497496
(errcode(ERRCODE_DATATYPE_MISMATCH),
498497
errmsg("cannot determine result data type"),
499-
errdetail("An aggregate returning a polymorphic type "
500-
"must have at least one polymorphic argument.")));
498+
errdetail_internal("%s", detailmsg)));
501499

502500
/*
503501
* Also, the return type can't be INTERNAL unless there's at least one
504502
* INTERNAL argument. This is the same type-safety restriction we enforce
505503
* for regular functions, but at the level of aggregates. We must test
506504
* this explicitly because we allow INTERNAL as the transtype.
507505
*/
508-
if (finaltype == INTERNALOID && !hasInternalArg)
506+
detailmsg = check_valid_internal_signature(finaltype,
507+
aggArgTypes,
508+
numArgs);
509+
if (detailmsg)
509510
ereport(ERROR,
510511
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
511512
errmsg("unsafe use of pseudo-type \"internal\""),
512-
errdetail("A function returning \"internal\" must have at least one \"internal\" argument.")));
513+
errdetail_internal("%s", detailmsg)));
513514

514515
/*
515516
* If a moving-aggregate implementation is supplied, look up its finalfn

‎src/backend/catalog/pg_proc.c

Lines changed: 45 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include "mb/pg_wchar.h"
3333
#include "miscadmin.h"
3434
#include "nodes/nodeFuncs.h"
35+
#include "parser/parse_coerce.h"
3536
#include "parser/parse_type.h"
3637
#include "tcop/pquery.h"
3738
#include "tcop/tcopprot.h"
@@ -97,12 +98,6 @@ ProcedureCreate(const char *procedureName,
9798
int allParamCount;
9899
Oid *allParams;
99100
char *paramModes = NULL;
100-
bool genericInParam = false;
101-
bool genericOutParam = false;
102-
bool anyrangeInParam = false;
103-
bool anyrangeOutParam = false;
104-
bool internalInParam = false;
105-
bool internalOutParam = false;
106101
Oid variadicType = InvalidOid;
107102
Acl *proacl = NULL;
108103
Relation rel;
@@ -116,6 +111,7 @@ ProcedureCreate(const char *procedureName,
116111
bool is_update;
117112
ObjectAddress myself,
118113
referenced;
114+
char *detailmsg;
119115
int i;
120116
Oid trfid;
121117

@@ -178,29 +174,34 @@ ProcedureCreate(const char *procedureName,
178174
}
179175

180176
/*
181-
* Detect whether we have polymorphic or INTERNAL arguments. The first
182-
* loop checks input arguments, the second output arguments.
177+
* Do not allow polymorphic return type unless there is a polymorphic
178+
* input argument that we can use to deduce the actual return type.
183179
*/
184-
for (i = 0; i < parameterCount; i++)
185-
{
186-
switch (parameterTypes->values[i])
187-
{
188-
case ANYARRAYOID:
189-
case ANYELEMENTOID:
190-
case ANYNONARRAYOID:
191-
case ANYENUMOID:
192-
genericInParam = true;
193-
break;
194-
case ANYRANGEOID:
195-
genericInParam = true;
196-
anyrangeInParam = true;
197-
break;
198-
case INTERNALOID:
199-
internalInParam = true;
200-
break;
201-
}
202-
}
180+
detailmsg = check_valid_polymorphic_signature(returnType,
181+
parameterTypes->values,
182+
parameterCount);
183+
if (detailmsg)
184+
ereport(ERROR,
185+
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
186+
errmsg("cannot determine result data type"),
187+
errdetail_internal("%s", detailmsg)));
203188

189+
/*
190+
* Also, do not allow return type INTERNAL unless at least one input
191+
* argument is INTERNAL.
192+
*/
193+
detailmsg = check_valid_internal_signature(returnType,
194+
parameterTypes->values,
195+
parameterCount);
196+
if (detailmsg)
197+
ereport(ERROR,
198+
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
199+
errmsg("unsafe use of pseudo-type \"internal\""),
200+
errdetail_internal("%s", detailmsg)));
201+
202+
/*
203+
* Apply the same tests to any OUT arguments.
204+
*/
204205
if (allParameterTypes != PointerGetDatum(NULL))
205206
{
206207
for (i = 0; i < allParamCount; i++)
@@ -210,52 +211,26 @@ ProcedureCreate(const char *procedureName,
210211
paramModes[i] == PROARGMODE_VARIADIC)
211212
continue; /* ignore input-only params */
212213

213-
switch (allParams[i])
214-
{
215-
case ANYARRAYOID:
216-
case ANYELEMENTOID:
217-
case ANYNONARRAYOID:
218-
case ANYENUMOID:
219-
genericOutParam = true;
220-
break;
221-
case ANYRANGEOID:
222-
genericOutParam = true;
223-
anyrangeOutParam = true;
224-
break;
225-
case INTERNALOID:
226-
internalOutParam = true;
227-
break;
228-
}
214+
detailmsg = check_valid_polymorphic_signature(allParams[i],
215+
parameterTypes->values,
216+
parameterCount);
217+
if (detailmsg)
218+
ereport(ERROR,
219+
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
220+
errmsg("cannot determine result data type"),
221+
errdetail_internal("%s", detailmsg)));
222+
detailmsg = check_valid_internal_signature(allParams[i],
223+
parameterTypes->values,
224+
parameterCount);
225+
if (detailmsg)
226+
ereport(ERROR,
227+
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
228+
errmsg("unsafe use of pseudo-type \"internal\""),
229+
errdetail_internal("%s", detailmsg)));
229230
}
230231
}
231232

232-
/*
233-
* Do not allow polymorphic return type unless at least one input argument
234-
* is polymorphic. ANYRANGE return type is even stricter: must have an
235-
* ANYRANGE input (since we can't deduce the specific range type from
236-
* ANYELEMENT). Also, do not allow return type INTERNAL unless at least
237-
* one input argument is INTERNAL.
238-
*/
239-
if ((IsPolymorphicType(returnType) || genericOutParam)
240-
&& !genericInParam)
241-
ereport(ERROR,
242-
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
243-
errmsg("cannot determine result data type"),
244-
errdetail("A function returning a polymorphic type must have at least one polymorphic argument.")));
245-
246-
if ((returnType == ANYRANGEOID || anyrangeOutParam) &&
247-
!anyrangeInParam)
248-
ereport(ERROR,
249-
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
250-
errmsg("cannot determine result data type"),
251-
errdetail("A function returning \"anyrange\" must have at least one \"anyrange\" argument.")));
252-
253-
if ((returnType == INTERNALOID || internalOutParam) && !internalInParam)
254-
ereport(ERROR,
255-
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
256-
errmsg("unsafe use of pseudo-type \"internal\""),
257-
errdetail("A function returning \"internal\" must have at least one \"internal\" argument.")));
258-
233+
/* Identify variadic argument type, if any */
259234
if (paramModes != NULL)
260235
{
261236
/*

‎src/backend/parser/parse_coerce.c

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1971,6 +1971,77 @@ enforce_generic_type_consistency(const Oid *actual_arg_types,
19711971
return rettype;
19721972
}
19731973

1974+
/*
1975+
* check_valid_polymorphic_signature()
1976+
* Is a proposed function signature valid per polymorphism rules?
1977+
*
1978+
* Returns NULL if the signature is valid (either ret_type is not polymorphic,
1979+
* or it can be deduced from the given declared argument types). Otherwise,
1980+
* returns a palloc'd, already translated errdetail string saying why not.
1981+
*/
1982+
char *
1983+
check_valid_polymorphic_signature(Oid ret_type,
1984+
const Oid *declared_arg_types,
1985+
int nargs)
1986+
{
1987+
if (ret_type == ANYRANGEOID)
1988+
{
1989+
/*
1990+
* ANYRANGE requires an ANYRANGE input, else we can't tell which of
1991+
* several range types with the same element type to use.
1992+
*/
1993+
for (int i = 0; i < nargs; i++)
1994+
{
1995+
if (declared_arg_types[i] == ret_type)
1996+
return NULL; /* OK */
1997+
}
1998+
return psprintf(_("A result of type %s requires at least one input of type %s."),
1999+
format_type_be(ret_type), format_type_be(ret_type));
2000+
}
2001+
else if (IsPolymorphicType(ret_type))
2002+
{
2003+
/* Otherwise, any polymorphic type can be deduced from any other */
2004+
for (int i = 0; i < nargs; i++)
2005+
{
2006+
if (IsPolymorphicType(declared_arg_types[i]))
2007+
return NULL; /* OK */
2008+
}
2009+
return psprintf(_("A result of type %s requires at least one input of type anyelement, anyarray, anynonarray, anyenum, or anyrange."),
2010+
format_type_be(ret_type));
2011+
}
2012+
else
2013+
return NULL; /* OK, ret_type is not polymorphic */
2014+
}
2015+
2016+
/*
2017+
* check_valid_internal_signature()
2018+
* Is a proposed function signature valid per INTERNAL safety rules?
2019+
*
2020+
* Returns NULL if OK, or a suitable error message if ret_type is INTERNAL but
2021+
* none of the declared arg types are. (It's unsafe to create such a function
2022+
* since it would allow invocation of INTERNAL-consuming functions directly
2023+
* from SQL.) It's overkill to return the error detail message, since there
2024+
* is only one possibility, but we do it like this to keep the API similar to
2025+
* check_valid_polymorphic_signature().
2026+
*/
2027+
char *
2028+
check_valid_internal_signature(Oid ret_type,
2029+
const Oid *declared_arg_types,
2030+
int nargs)
2031+
{
2032+
if (ret_type == INTERNALOID)
2033+
{
2034+
for (int i = 0; i < nargs; i++)
2035+
{
2036+
if (declared_arg_types[i] == ret_type)
2037+
return NULL; /* OK */
2038+
}
2039+
return pstrdup(_("A result of type internal requires at least one input of type internal."));
2040+
}
2041+
else
2042+
return NULL; /* OK, ret_type is not INTERNAL */
2043+
}
2044+
19742045

19752046
/* TypeCategory()
19762047
* Assign a category to the specified type OID.

‎src/include/parser/parse_coerce.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,13 @@ extern Oid enforce_generic_type_consistency(const Oid *actual_arg_types,
8080
Oid rettype,
8181
bool allow_poly);
8282

83+
extern char *check_valid_polymorphic_signature(Oid ret_type,
84+
const Oid *declared_arg_types,
85+
int nargs);
86+
extern char *check_valid_internal_signature(Oid ret_type,
87+
const Oid *declared_arg_types,
88+
int nargs);
89+
8390
extern CoercionPathType find_coercion_pathway(Oid targetTypeId,
8491
Oid sourceTypeId,
8592
CoercionContext ccontext,

‎src/test/regress/expected/plpgsql.out

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1811,7 +1811,7 @@ begin
18111811
return array[x + 1, x + 2];
18121812
end$$ language plpgsql;
18131813
ERROR: cannot determine result data type
1814-
DETAIL: A function returning "anyrange" must have at least one "anyrange" argument.
1814+
DETAIL: A result of type anyrange requires at least one input of type anyrange.
18151815
create function f1(x anyrange) returns anyarray as $$
18161816
begin
18171817
return array[lower(x), upper(x)];

0 commit comments

Comments
 (0)