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

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.

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.

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