Re: checking variadic "any" argument in parser - should be array

Hello

2013/6/27 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)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.

>
> 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?

> 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 ???

Regards

Pavel

>
> Thanks
>
>
>
> On Thu, Jun 27, 2013 at 12:35 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>>
>> Hello
>>
>> remastered version
>>
>> Regards
>>
>> Pavel
>>
>> 2013/6/26 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>:
>> > Hi Pavel
>> >
>> >
>> > On Sat, Jan 26, 2013 at 9:22 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)postgresql(dot)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.