Re: checking variadic "any" argument in parser - should be array - Mailing list pgsql-hackers

<div dir="ltr">Hi Pavel,<br /><br />I had a look over your new patch and it looks good to me.<br /><br />My review
commentson patch:<br /><br />1. It cleanly applies with patch -p1 command.<br />2. make/make install/make check were
smooth.<br/> 3. My own testing didn't find any issue.<br />4. I had a code walk-through and I am little bit worried or
confusedon<br />following changes:<br /><br /><font size="1"><span style="font-family:courier new,monospace">         
if(PG_ARGISNULL(argidx))<br />               return NULL;<br />  <br />!         /*<br />!          * Non-null argument
hadbetter be an array.  The parser doesn't<br />!          * enforce this for VARIADIC ANY functions (maybe it
should?),so that<br />!          * check uses ereport not just elog.<br /> !          */<br />!         arr_typid =
get_fn_expr_argtype(fcinfo->flinfo,argidx);<br />!         if (!OidIsValid(arr_typid))<br />!            
elog(ERROR,"could not determine data type of concat() input");<br /> ! <br />!         if
(!OidIsValid(get_element_type(arr_typid)))<br/>!             ereport(ERROR,<br />!                    
(errcode(ERRCODE_DATATYPE_MISMATCH),<br/>!                      errmsg("VARIADIC argument must be an array")));<br />  
<br/>-         /* OK, safe to fetch the array value */<br />          arr = PG_GETARG_ARRAYTYPE_P(argidx);<br />  <br
/>         /*<br />--- 3820,3828 ----<br />          if (PG_ARGISNULL(argidx))<br />              return NULL;<br />  
<br/>!         /* Non-null argument had better be an array */<br />!        
Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo,argidx))));<br />  <br />          arr =
PG_GETARG_ARRAYTYPE_P(argidx);<br/></span></font><br />We have moved checking of array type in parser
(ParseFuncOrColumn())which<br />basically verifies that argument type is indeed an array. Which exactly same<br />as
thatof second if statement in earlier code, which you replaced by an<br /> Assert.<br /><br />However, what about first
ifstatement ? Is it NO more required now? What if<br />get_fn_expr_argtype() returns InvalidOid, don't you think we
needto throw<br />an error saying "could not determine data type of concat() input"?<br /><br />Well, I tried hard to
triggerthat code, but didn't able to get any test<br />which fails with that error in earlier version and with your
version.And<br />thus I believe it is a dead code, which you removed ? Is it so ?<br /><br />Moreover, if in any case
get_fn_expr_argtype()returns an InvalidOid, we<br />will hit an Assert rather than an error, is this what you expect
?<br/><br />5. This patch has user visibility, i.e. now we are throwing an error when<br /> user only says "VARIADIC
NULL"like:<br /><br /><span style="font-family:courier new,monospace">    select concat(variadic NULL) is
NULL;</span><br/><br />Previously it was working but now we are throwing an error. Well we are now<br /> more stricter
thanearlier with using VARIADIC + ANY, so I have no issue as<br />such. But I guess we need to document this user
visibilitychange. I don't<br />know exactly where though. I searched for VARIADIC and all related<br /> documentation
saysit needs an array, so nothing harmful as such, so you can<br />ignore this review comment but I thought it worth
mentioningit.<br /><br />Thanks<br /><div class="gmail_extra"><br /><br /><div class="gmail_quote"> On Thu, Jun 27,
2013at 12:35 AM, Pavel Stehule <span dir="ltr"><<a href="mailto:pavel.stehule@gmail.com"
target="_blank">pavel.stehule@gmail.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0px 0px
0px0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello<br /><br /> remastered version<br /><br />
Regards<br/><br /> Pavel<br /><br /> 2013/6/26 Jeevan Chalke <<a
href="mailto:jeevan.chalke@enterprisedb.com">jeevan.chalke@enterprisedb.com</a>>:<br/><div class=""><div
class="h5">>Hi Pavel<br /> ><br /> ><br /> > On Sat, Jan 26, 2013 at 9:22 AM, Pavel Stehule <<a
href="mailto:pavel.stehule@gmail.com">pavel.stehule@gmail.com</a>><br/> > wrote:<br /> >><br /> >>
HelloTom<br /> >><br /> >> you did comment<br /> >><br /> >> !
<----><------><------>* Non-null argument had better be an array.<br /> >> The parser
doesn't<br/> >> ! <----><------><------> * enforce this for VARIADIC ANY functions<br />
>>(maybe it should?), so<br /> >> ! <----><------><------> * that check uses ereport not
justelog.<br /> >> ! <----><------><------> */<br /> >><br /> >> So I moved this
checkto parser.<br /> >><br /> >> It is little bit stricter - requests typed NULL instead unknown NULL,<br
/>>> but it can mark error better and early<br /> ><br /> ><br /> > Tom suggested that this check should
bebetter done by parser.<br /> > This patch tries to accomplish that.<br /> ><br /> > I will go review
this.<br/> ><br /> > However, is it possible to you to re-base it on current master? I can't<br /> > apply it
using"git apply" but patch -p1 was succeeded with lot of offset.<br /> ><br /> > Thanks<br /> ><br />
>><br/> >><br /> >> Regards<br /> >><br /> >> Pavel<br /> >><br /> >><br />
>>--<br /> >> Sent via pgsql-hackers mailing list (<a
href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br/> >> To make changes to your
subscription:<br/> >> <a href="http://www.postgresql.org/mailpref/pgsql-hackers"
target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/> >><br /> ><br /> ><br /> ><br
/>> --<br /> > Jeevan B Chalke<br /> > Senior Software Engineer, R&D<br /> > EnterpriseDB
Corporation<br/> > The Enterprise PostgreSQL Company<br /> ><br /> > Phone: +91 20 30589500<br /> ><br />
>Website: <a href="http://www.enterprisedb.com" target="_blank">www.enterprisedb.com</a><br /> > EnterpriseDB
Blog:<a href="http://blogs.enterprisedb.com/" target="_blank">http://blogs.enterprisedb.com/</a><br /> > Follow us
onTwitter: <a href="http://www.twitter.com/enterprisedb" target="_blank">http://www.twitter.com/enterprisedb</a><br />
><br/> > This e-mail message (and any attachment) is intended for the use of the<br /> > individual or entity
towhom it is addressed. This message contains<br /> > information from EnterpriseDB Corporation that may be
privileged,<br/> > confidential, or exempt from disclosure under applicable law. If you are not<br /> > the
intendedrecipient or authorized to receive this for the intended<br /> > recipient, any use, dissemination,
distribution,retention, archiving, or<br /> > copying of this communication is strictly prohibited. If you have
received<br/> > this e-mail in error, please notify the sender immediately by reply e-mail<br /> > and delete
thismessage.<br /></div></div></blockquote></div><br /><br clear="all" /><br />-- <br />Jeevan B Chalke<br />Senior
SoftwareEngineer, R&D<br />EnterpriseDB Corporation<br />The Enterprise PostgreSQL Company<br /><br />Phone: +91 20
30589500<br/><br />Website: <a href="http://www.enterprisedb.com" target="_blank">www.enterprisedb.com</a><br />
EnterpriseDBBlog: <a href="http://blogs.enterprisedb.com/" target="_blank">http://blogs.enterprisedb.com/</a><br
/>Followus on Twitter: <a href="http://www.twitter.com/enterprisedb"
target="_blank">http://www.twitter.com/enterprisedb</a><br/><br />This e-mail message (and any attachment) is intended
forthe use of the individual or entity to whom it is addressed. This message contains information from EnterpriseDB
Corporationthat may be privileged, confidential, or exempt from disclosure under applicable law. If you are not the
intendedrecipient 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. </div></div>