[prev in list] [next in list] [prev in thread] [next in thread]
List: postgresql-general
Subject: Re: [HACKERS] checking variadic "any" argument in parser - should be array
From: Pavel Stehule <pavel.stehule () gmail ! com>
Date: 2013-06-29 19:29:23
Message-ID: CAFj8pRDe_M-O7a06yZXgEk-LBVApFu2FrJgAkwCfPxNi8pYSig () mail ! gmail ! com
[Download RAW message or body]
Hello
updated patch - precious Assert, more comments
Regards
Pavel
2013/6/29 Pavel Stehule <pavel.stehule@gmail.com>:
> 2013/6/28 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:
>> Hi Pavel,
>>
>>
>> On Fri, Jun 28, 2013 at 2:53 AM, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>>>
>>> Hello
>>>
>>> 2013/6/27 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:
>>> > Hi Pavel,
>>> >
>>> > I had a look over your new patch and it looks good to me.
>>> >
>>> > My review comments on patch:
>>> >
>>> > 1. It cleanly applies with patch -p1 command.
>>> > 2. make/make install/make check were smooth.
>>> > 3. My own testing didn't find any issue.
>>> > 4. I had a code walk-through and I am little bit worried or confused on
>>> > following changes:
>>> >
>>> > if (PG_ARGISNULL(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.
>>> > ! */
>>> > ! 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")));
>>> >
>>> > - /* OK, safe to fetch the array value */
>>> > arr = PG_GETARG_ARRAYTYPE_P(argidx);
>>> >
>>> > /*
>>> > --- 3820,3828 ----
>>> > if (PG_ARGISNULL(argidx))
>>> > return NULL;
>>> >
>>> > ! /* Non-null argument had better be an array */
>>> > !
>>> > Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo,
>>> > argidx))));
>>> >
>>> > arr = PG_GETARG_ARRAYTYPE_P(argidx);
>>> >
>>> > We have moved checking of array type in parser (ParseFuncOrColumn())
>>> > which
>>> > basically verifies that argument type is indeed an array. Which exactly
>>> > same
>>> > as that of second if statement in earlier code, which you replaced by an
>>> > Assert.
>>> >
>>> > However, what about first if statement ? Is it NO more required now?
>>> > What if
>>> > get_fn_expr_argtype() returns InvalidOid, don't you think we need to
>>> > throw
>>> > an error saying "could not determine data type of concat() input"?
>>>
>>> yes, If I understand well to question, a main differences is in stage
>>> of checking. When I do a check in parser stage, then I can expect so
>>> "actual_arg_types" array holds a valid values.
>>
>>
>> That's fine.
>>
>>>
>>>
>>> >
>>> > Well, I tried hard to trigger that code, but didn't able to get any test
>>> > which fails with that error in earlier version and with your version.
>>> > And
>>> > thus I believe it is a dead code, which you removed ? Is it so ?
>>>
>>> It is removed in this version :), and it is not a bug, so there is not
>>> reason for patching previous versions. Probably there should be a
>>> Assert instead of error. This code is relatively mature - so I don't
>>> expect a issue from SQL level now. On second hand, this functions can
>>> be called via DirectFunctionCall API from custom C server side
>>> routines, and there a developer can does a bug simply if doesn't fill
>>> necessary structs well. So, there can be Asserts still.
>>>
>>> >
>>> > Moreover, if in any case get_fn_expr_argtype() returns an InvalidOid, we
>>> > will hit an Assert rather than an error, is this what you expect ?
>>> >
>>>
>>> in this moment yes,
>>>
>>> small change can helps with searching of source of possible issues.
>>>
>>> so instead on line
>>> Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo,
>>> argidx))));
>>>
>>> use two lines
>>>
>>> Assert(OidIsValid(get_fn_expr_argtype(fcinfo->flinfo, argidx)));
>>> Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo,
>>> argidx))));
>>>
>>> what you think?
>>
>>
>> Well, I am still not fully understand or convinced about first Assert, error
>> will be good enough like what we have now.
>>
>> Anyway, converting it over two lines eases the debugging efforts. But please
>> take output of get_fn_expr_argtype(fcinfo->flinfo, argidx) into separate
>> variable so that we will avoid calling same function twice.
>
> It is called in Assert, so it will be removed in production
> environment. Using variable for this purpose is useless and less
> maintainable.
>
>>
>> I think some short comment for these Asserts will be good. At-least for
>> second one as it is already done by parser and non-arrays are not at
>> expected at this point.
>>
>
> yes, I'll add some comment
>
> Regards
>
> Pavel
>
>
>>>
>>> > 5. This patch has user visibility, i.e. now we are throwing an error
>>> > when
>>> > user only says "VARIADIC NULL" like:
>>> >
>>> > select concat(variadic NULL) is NULL;
>>> >
>>> > Previously it was working but now we are throwing an error. Well we are
>>> > now
>>> > more stricter than earlier with using VARIADIC + ANY, so I have no issue
>>> > as
>>> > such. But I guess we need to document this user visibility change. I
>>> > don't
>>> > know exactly where though. I searched for VARIADIC and all related
>>> > documentation says it needs an array, so nothing harmful as such, so you
>>> > can
>>> > ignore this review comment but I thought it worth mentioning it.
>>>
>>> yes, it is point for possible issues in RELEASE NOTES, I am thinking ???
>>>
>>
>> Well, writer of release notes should be aware of this. And I hope he will
>> be. So no issue.
>>
>> Thanks
>>
>>>
>>> Regards
>>>
>>> Pavel
>>>
>>> >
>>> > Thanks
>>> >
>>> >
>>> >
>>> > On Thu, Jun 27, 2013 at 12:35 AM, Pavel Stehule
>>> > <pavel.stehule@gmail.com>
>>> > wrote:
>>> >>
>>> >> Hello
>>> >>
>>> >> remastered version
>>> >>
>>> >> Regards
>>> >>
>>> >> Pavel
>>> >>
>>> >> 2013/6/26 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:
>>> >> > Hi Pavel
>>> >> >
>>> >> >
>>> >> > On Sat, Jan 26, 2013 at 9:22 AM, Pavel Stehule
>>> >> > <pavel.stehule@gmail.com>
>>> >> > wrote:
>>> >> >>
>>> >> >> Hello Tom
>>> >> >>
>>> >> >> you did comment
>>> >> >>
>>> >> >> ! <----><------><------> * 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.
>>> >> >> ! <----><------><------> */
>>> >> >>
>>> >> >> So I moved this check to parser.
>>> >> >>
>>> >> >> It is little bit stricter - requests typed NULL instead unknown
>>> >> >> NULL,
>>> >> >> but it can mark error better and early
>>> >> >
>>> >> >
>>> >> > Tom suggested that this check should be better done by parser.
>>> >> > This patch tries to accomplish that.
>>> >> >
>>> >> > I will go review this.
>>> >> >
>>> >> > However, is it possible to you to re-base it on current master? I
>>> >> > can't
>>> >> > apply it using "git apply" but patch -p1 was succeeded with lot of
>>> >> > offset.
>>> >> >
>>> >> > Thanks
>>> >> >
>>> >> >>
>>> >> >>
>>> >> >> Regards
>>> >> >>
>>> >> >> Pavel
>>> >> >>
>>> >> >>
>>> >> >> --
>>> >> >> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>>> >> >> To make changes to your subscription:
>>> >> >> http://www.postgresql.org/mailpref/pgsql-hackers
>>> >> >>
>>> >> >
>>> >> >
>>> >> >
>>> >> > --
>>> >> > Jeevan B Chalke
>>> >> > Senior Software Engineer, R&D
>>> >> > EnterpriseDB Corporation
>>> >> > The Enterprise PostgreSQL Company
>>> >> >
>>> >> > Phone: +91 20 30589500
>>> >> >
>>> >> > Website: www.enterprisedb.com
>>> >> > EnterpriseDB Blog: http://blogs.enterprisedb.com/
>>> >> > Follow us on Twitter: http://www.twitter.com/enterprisedb
>>> >> >
>>> >> > This e-mail message (and any attachment) is intended for the use of
>>> >> > the
>>> >> > individual or entity to whom it is addressed. This message contains
>>> >> > information from EnterpriseDB Corporation that may be privileged,
>>> >> > confidential, or exempt from disclosure under applicable law. If you
>>> >> > are
>>> >> > not
>>> >> > the intended recipient or authorized to receive this for the intended
>>> >> > recipient, any use, dissemination, distribution, retention,
>>> >> > archiving,
>>> >> > or
>>> >> > copying of this communication is strictly prohibited. If you have
>>> >> > received
>>> >> > this e-mail in error, please notify the sender immediately by reply
>>> >> > e-mail
>>> >> > and delete this message.
>>> >
>>> >
>>> >
>>> >
>>> > --
>>> > Jeevan B Chalke
>>> > Senior Software Engineer, R&D
>>> > EnterpriseDB Corporation
>>> > The Enterprise PostgreSQL Company
>>> >
>>> > Phone: +91 20 30589500
>>> >
>>> > Website: www.enterprisedb.com
>>> > EnterpriseDB Blog: http://blogs.enterprisedb.com/
>>> > Follow us on Twitter: http://www.twitter.com/enterprisedb
>>> >
>>> > This e-mail message (and any attachment) is intended for the use of the
>>> > individual or entity to whom it is addressed. This message contains
>>> > information from EnterpriseDB Corporation that may be privileged,
>>> > confidential, or exempt from disclosure under applicable law. If you are
>>> > not
>>> > the intended recipient or authorized to receive this for the intended
>>> > recipient, any use, dissemination, distribution, retention, archiving,
>>> > or
>>> > copying of this communication is strictly prohibited. If you have
>>> > received
>>> > this e-mail in error, please notify the sender immediately by reply
>>> > e-mail
>>> > and delete this message.
>>
>>
>>
>>
>> --
>> Jeevan B Chalke
>> Senior Software Engineer, R&D
>> EnterpriseDB Corporation
>> The Enterprise PostgreSQL Company
>>
>> Phone: +91 20 30589500
>>
>> Website: www.enterprisedb.com
>> EnterpriseDB Blog: http://blogs.enterprisedb.com/
>> Follow us on Twitter: http://www.twitter.com/enterprisedb
>>
>> This e-mail message (and any attachment) is intended for the use of the
>> individual or entity to whom it is addressed. This message contains
>> information from EnterpriseDB Corporation that may be privileged,
>> confidential, or exempt from disclosure under applicable law. If you are not
>> the intended recipient or authorized to receive this for the intended
>> recipient, any use, dissemination, distribution, retention, archiving, or
>> copying of this communication is strictly prohibited. If you have received
>> this e-mail in error, please notify the sender immediately by reply e-mail
>> and delete this message.
["variadic_any_parser_check-3.patch" (application/octet-stream)]
*** a/src/backend/catalog/pg_aggregate.c
--- b/src/backend/catalog/pg_aggregate.c
***************
*** 332,337 **** lookup_agg_function(List *fnName,
--- 332,338 ----
Oid fnOid;
bool retset;
int nvargs;
+ Oid vatype;
Oid *true_oid_array;
FuncDetailCode fdresult;
AclResult aclresult;
***************
*** 346,352 **** lookup_agg_function(List *fnName,
*/
fdresult = func_get_detail(fnName, NIL, NIL,
nargs, input_types, false, false,
! &fnOid, rettype, &retset, &nvargs,
&true_oid_array, NULL);
/* only valid case is a normal function not returning a set */
--- 347,354 ----
*/
fdresult = func_get_detail(fnName, NIL, NIL,
nargs, input_types, false, false,
! &fnOid, rettype, &retset,
! &nvargs, &vatype,
&true_oid_array, NULL);
/* only valid case is a normal function not returning a set */
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
***************
*** 79,84 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
--- 79,85 ----
Node *retval;
bool retset;
int nvargs;
+ Oid vatype;
FuncDetailCode fdresult;
/*
***************
*** 214,220 **** 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,
&declared_arg_types, &argdefaults);
if (fdresult == FUNCDETAIL_COERCION)
{
--- 215,222 ----
fdresult = func_get_detail(funcname, fargs, argnames, nargs,
actual_arg_types,
!func_variadic, true,
! &funcid, &rettype, &retset,
! &nvargs, &vatype,
&declared_arg_types, &argdefaults);
if (fdresult == FUNCDETAIL_COERCION)
{
***************
*** 376,381 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
--- 378,399 ----
fargs = lappend(fargs, newa);
}
+ /*
+ * When function is called with VARIADIC labeled parameter,
+ * and declared_arg_type is "ANY", then sanitize a real parameter
+ * type now - should 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)
{
***************
*** 1015,1020 **** func_get_detail(List *funcname,
--- 1033,1039 ----
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 */
{
***************
*** 1233,1238 **** func_get_detail(List *funcname,
--- 1252,1258 ----
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)
{
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***************
*** 8570,8575 **** generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
--- 8570,8576 ----
Oid p_rettype;
bool p_retset;
int p_nvargs;
+ Oid p_vatype;
Oid *p_true_typeids;
proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
***************
*** 8620,8626 **** 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);
if ((p_result == FUNCDETAIL_NORMAL ||
p_result == FUNCDETAIL_AGGREGATE ||
p_result == FUNCDETAIL_WINDOWFUNC) &&
--- 8621,8628 ----
NIL, argnames, nargs, argtypes,
!use_variadic, true,
&p_funcid, &p_rettype,
! &p_retset, &p_nvargs, &p_vatype,
! &p_true_typeids, NULL);
if ((p_result == FUNCDETAIL_NORMAL ||
p_result == FUNCDETAIL_AGGREGATE ||
p_result == FUNCDETAIL_WINDOWFUNC) &&
*** a/src/backend/utils/adt/varlena.c
--- b/src/backend/utils/adt/varlena.c
***************
*** 3820,3826 **** 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 */
--- 3820,3825 ----
***************
*** 3831,3850 **** 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.
*/
! 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")));
- /* OK, safe to fetch the array value */
arr = PG_GETARG_ARRAYTYPE_P(argidx);
/*
--- 3830,3845 ----
return NULL;
/*
! * Non-null argument had better be an array
! *
! * Correct values are ensured by parser check, but this functionality
! * can be called directly with bypassing a parser, so we should do
! * some minimal check too - this form of call require correctly set
! * expr argtype in flinfo.
*/
! Assert(OidIsValid(get_fn_expr_argtype(fcinfo->flinfo, argidx)));
! Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo, argidx))));
arr = PG_GETARG_ARRAYTYPE_P(argidx);
/*
***************
*** 4049,4055 **** 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;
--- 4044,4049 ----
***************
*** 4065,4084 **** 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.
*/
! 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")));
- /* OK, safe to fetch the array value */
arr = PG_GETARG_ARRAYTYPE_P(1);
/* Get info about array element type */
--- 4059,4074 ----
else
{
/*
! * Non-null argument had better be an array
! *
! * Correct values are ensured by parser check, but this functionality
! * can be called directly with bypassing a parser, so we should do
! * some minimal check too - this form of call require correctly set
! * expr argtype in flinfo.
*/
! Assert(OidIsValid(get_fn_expr_argtype(fcinfo->flinfo, 1)));
! Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo, 1))));
arr = PG_GETARG_ARRAYTYPE_P(1);
/* Get info about array element type */
*** a/src/include/parser/parse_func.h
--- b/src/include/parser/parse_func.h
***************
*** 53,60 **** 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);
extern int func_match_argtypes(int nargs,
Oid *input_typeids,
--- 53,60 ----
int nargs, Oid *argtypes,
bool expand_variadic, bool expand_defaults,
Oid *funcid, Oid *rettype,
! bool *retset, int *nvargs, Oid *vatype,
! Oid **true_typeids, List **argdefaults);
extern int func_match_argtypes(int nargs,
Oid *input_typeids,
*** a/src/test/regress/expected/text.out
--- b/src/test/regress/expected/text.out
***************
*** 149,161 **** select concat_ws(',', variadic array[1,2,3]);
1,2,3
(1 row)
! select concat_ws(',', variadic NULL);
concat_ws
-----------
(1 row)
! select concat(variadic NULL) is NULL;
?column?
----------
t
--- 149,161 ----
1,2,3
(1 row)
! select concat_ws(',', variadic NULL::int[]);
concat_ws
-----------
(1 row)
! select concat(variadic NULL::int[]) is NULL;
?column?
----------
t
***************
*** 170,175 **** select concat(variadic '{}'::int[]) = '';
--- 170,177 ----
--should fail
select concat_ws(',', variadic 10);
ERROR: VARIADIC argument must be an array
+ LINE 1: select concat_ws(',', variadic 10);
+ ^
/*
* format
*/
***************
*** 315,322 **** 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);
format
--------
Hello
--- 317,324 ----
2, 1
(1 row)
! -- variadic argument can be array type NULL, but should not be referenced
! select format('Hello', variadic NULL::int[]);
format
--------
Hello
*** a/src/test/regress/sql/text.sql
--- b/src/test/regress/sql/text.sql
***************
*** 47,54 **** 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(variadic '{}'::int[]) = '';
--should fail
select concat_ws(',', variadic 10);
--- 47,54 ----
-- check variadic labeled argument
select concat(variadic array[1,2,3]);
select concat_ws(',', variadic array[1,2,3]);
! 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,100 **** 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 allows simulating more than FUNC_MAX_ARGS parameters
select format(string_agg('%s',','), variadic array_agg(i))
from generate_series(1,200) g(i);
--- 93,100 ----
-- 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 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);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic