From d26888bc4d1e539a82f21382b0000fe5bbf889d9 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Thu, 18 Jul 2013 11:52:12 -0400 Subject: [PATCH] Move checking an explicit VARIADIC "any" argument into the parser. This is more efficient and simpler . It does mean that an untyped NULL can no longer be used in such cases, which should be mentioned in Release Notes, but doesn't seem a terrible loss. The workaround is to cast the NULL to some array type. Pavel Stehule, reviewed by Jeevan Chalke. --- src/backend/catalog/pg_aggregate.c | 4 ++- src/backend/parser/parse_func.c | 22 +++++++++++++++- src/backend/utils/adt/ruleutils.c | 4 ++- src/backend/utils/adt/varlena.c | 42 ++++++++++++------------------ src/include/parser/parse_func.h | 4 +-- src/test/regress/expected/text.out | 10 ++++--- src/test/regress/sql/text.sql | 8 +++--- 7 files changed, 55 insertions(+), 39 deletions(-) diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c index e80b60053d..480c17cd39 100644 --- a/src/backend/catalog/pg_aggregate.c +++ b/src/backend/catalog/pg_aggregate.c @@ -332,6 +332,7 @@ lookup_agg_function(List *fnName, Oid fnOid; bool retset; int nvargs; + Oid vatype; Oid *true_oid_array; FuncDetailCode fdresult; AclResult aclresult; @@ -346,7 +347,8 @@ lookup_agg_function(List *fnName, */ fdresult = func_get_detail(fnName, NIL, NIL, nargs, input_types, false, false, - &fnOid, rettype, &retset, &nvargs, + &fnOid, rettype, &retset, + &nvargs, &vatype, &true_oid_array, NULL); /* only valid case is a normal function not returning a set */ diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index e54922f803..1f02c9a575 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -79,6 +79,7 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, Node *retval; bool retset; int nvargs; + Oid vatype; FuncDetailCode fdresult; /* @@ -214,7 +215,8 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, fdresult = func_get_detail(funcname, fargs, argnames, nargs, actual_arg_types, !func_variadic, true, - &funcid, &rettype, &retset, &nvargs, + &funcid, &rettype, &retset, + &nvargs, &vatype, &declared_arg_types, &argdefaults); if (fdresult == FUNCDETAIL_COERCION) { @@ -382,6 +384,22 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, fargs = lappend(fargs, newa); } + /* + * When function is called an explicit VARIADIC labeled parameter, + * and the declared_arg_type is "any", then sanity check the actual + * parameter type now - it must be an array. + */ + if (nargs > 0 && vatype == ANYOID && func_variadic) + { + Oid va_arr_typid = actual_arg_types[nargs - 1]; + + if (!OidIsValid(get_element_type(va_arr_typid))) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("VARIADIC argument must be an array"), + parser_errposition(pstate, exprLocation((Node *) llast(fargs))))); + } + /* build the appropriate output structure */ if (fdresult == FUNCDETAIL_NORMAL) { @@ -1033,6 +1051,7 @@ func_get_detail(List *funcname, Oid *rettype, /* return value */ bool *retset, /* return value */ int *nvargs, /* return value */ + Oid *vatype, /* return value */ Oid **true_typeids, /* return value */ List **argdefaults) /* optional return value */ { @@ -1251,6 +1270,7 @@ func_get_detail(List *funcname, pform = (Form_pg_proc) GETSTRUCT(ftup); *rettype = pform->prorettype; *retset = pform->proretset; + *vatype = pform->provariadic; /* fetch default args if caller wants 'em */ if (argdefaults && best_candidate->ndargs > 0) { diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 976bc98e37..40b565a11d 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -8584,6 +8584,7 @@ generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes, Oid p_rettype; bool p_retset; int p_nvargs; + Oid p_vatype; Oid *p_true_typeids; proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid)); @@ -8634,7 +8635,8 @@ generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes, NIL, argnames, nargs, argtypes, !use_variadic, true, &p_funcid, &p_rettype, - &p_retset, &p_nvargs, &p_true_typeids, NULL); + &p_retset, &p_nvargs, &p_vatype, + &p_true_typeids, NULL); if ((p_result == FUNCDETAIL_NORMAL || p_result == FUNCDETAIL_AGGREGATE || p_result == FUNCDETAIL_WINDOWFUNC) && diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 56349e7e2a..4aa344896f 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -3820,7 +3820,6 @@ concat_internal(const char *sepstr, int argidx, */ if (get_fn_expr_variadic(fcinfo->flinfo)) { - Oid arr_typid; ArrayType *arr; /* Should have just the one argument */ @@ -3831,20 +3830,16 @@ concat_internal(const char *sepstr, int argidx, return NULL; /* - * Non-null argument had better be an array. The parser doesn't - * enforce this for VARIADIC ANY functions (maybe it should?), so that - * check uses ereport not just elog. + * Non-null argument had better be an array + * + * Correct values are ensured by parser check, but this function + * can be called directly, bypassing the parser, so we should do + * some minimal check too - this form of call requires correctly set + * expr argtype in flinfo. */ - arr_typid = get_fn_expr_argtype(fcinfo->flinfo, argidx); - if (!OidIsValid(arr_typid)) - elog(ERROR, "could not determine data type of concat() input"); - - if (!OidIsValid(get_element_type(arr_typid))) - ereport(ERROR, - (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("VARIADIC argument must be an array"))); + Assert(OidIsValid(get_fn_expr_argtype(fcinfo->flinfo, argidx))); + Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo, argidx)))); - /* OK, safe to fetch the array value */ arr = PG_GETARG_ARRAYTYPE_P(argidx); /* @@ -4049,7 +4044,6 @@ text_format(PG_FUNCTION_ARGS) /* If argument is marked VARIADIC, expand array into elements */ if (get_fn_expr_variadic(fcinfo->flinfo)) { - Oid arr_typid; ArrayType *arr; int16 elmlen; bool elmbyval; @@ -4065,20 +4059,16 @@ text_format(PG_FUNCTION_ARGS) else { /* - * Non-null argument had better be an array. The parser doesn't - * enforce this for VARIADIC ANY functions (maybe it should?), so - * that check uses ereport not just elog. + * Non-null argument had better be an array + * + * Correct values are ensured by parser check, but this function + * can be called directly, bypassing the parser, so we should do + * some minimal check too - this form of call requires correctly set + * expr argtype in flinfo. */ - arr_typid = get_fn_expr_argtype(fcinfo->flinfo, 1); - if (!OidIsValid(arr_typid)) - elog(ERROR, "could not determine data type of format() input"); - - if (!OidIsValid(get_element_type(arr_typid))) - ereport(ERROR, - (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("VARIADIC argument must be an array"))); + Assert(OidIsValid(get_fn_expr_argtype(fcinfo->flinfo, 1))); + Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo, 1)))); - /* OK, safe to fetch the array value */ arr = PG_GETARG_ARRAYTYPE_P(1); /* Get info about array element type */ diff --git a/src/include/parser/parse_func.h b/src/include/parser/parse_func.h index d63cb95b34..d33eef3482 100644 --- a/src/include/parser/parse_func.h +++ b/src/include/parser/parse_func.h @@ -52,8 +52,8 @@ extern FuncDetailCode func_get_detail(List *funcname, int nargs, Oid *argtypes, bool expand_variadic, bool expand_defaults, Oid *funcid, Oid *rettype, - bool *retset, int *nvargs, Oid **true_typeids, - List **argdefaults); + bool *retset, int *nvargs, Oid *vatype, + Oid **true_typeids, List **argdefaults); extern int func_match_argtypes(int nargs, Oid *input_typeids, diff --git a/src/test/regress/expected/text.out b/src/test/regress/expected/text.out index 4b1c62bf53..1587046d95 100644 --- a/src/test/regress/expected/text.out +++ b/src/test/regress/expected/text.out @@ -149,13 +149,13 @@ select concat_ws(',', variadic array[1,2,3]); 1,2,3 (1 row) -select concat_ws(',', variadic NULL); +select concat_ws(',', variadic NULL::int[]); concat_ws ----------- (1 row) -select concat(variadic NULL) is NULL; +select concat(variadic NULL::int[]) is NULL; ?column? ---------- t @@ -170,6 +170,8 @@ select concat(variadic '{}'::int[]) = ''; --should fail select concat_ws(',', variadic 10); ERROR: VARIADIC argument must be an array +LINE 1: select concat_ws(',', variadic 10); + ^ /* * format */ @@ -315,8 +317,8 @@ select format('%2$s, %1$s', variadic array[1, 2]); 2, 1 (1 row) --- variadic argument can be NULL, but should not be referenced -select format('Hello', variadic NULL); +-- variadic argument can be array type NULL, but should not be referenced +select format('Hello', variadic NULL::int[]); format -------- Hello diff --git a/src/test/regress/sql/text.sql b/src/test/regress/sql/text.sql index c4ed74b39d..60c15b54c0 100644 --- a/src/test/regress/sql/text.sql +++ b/src/test/regress/sql/text.sql @@ -47,8 +47,8 @@ select quote_literal(e'\\'); -- check variadic labeled argument select concat(variadic array[1,2,3]); select concat_ws(',', variadic array[1,2,3]); -select concat_ws(',', variadic NULL); -select concat(variadic NULL) is NULL; +select concat_ws(',', variadic NULL::int[]); +select concat(variadic NULL::int[]) is NULL; select concat(variadic '{}'::int[]) = ''; --should fail select concat_ws(',', variadic 10); @@ -93,8 +93,8 @@ select format('%s, %s', variadic array[true, false]::text[]); -- check variadic with positional placeholders select format('%2$s, %1$s', variadic array['first', 'second']); select format('%2$s, %1$s', variadic array[1, 2]); --- variadic argument can be NULL, but should not be referenced -select format('Hello', variadic NULL); +-- variadic argument can be array type NULL, but should not be referenced +select format('Hello', variadic NULL::int[]); -- variadic argument allows simulating more than FUNC_MAX_ARGS parameters select format(string_agg('%s',','), variadic array_agg(i)) from generate_series(1,200) g(i); -- 2.40.0