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

Hello

updated patch - precious Assert, more comments

Regards

Pavel

2013/6/29 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> 2013/6/28 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>:
>> Hi Pavel,
>>
>>
>> On Fri, Jun 28, 2013 at 2:53 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>>>
>>> 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.
>>
>>
>> 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(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.
>>
>>
>>
>>
>> --
>> 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.