Re: checking variadic "any" argument in parser - should be array
От | Pavel Stehule |
---|---|
Тема | Re: checking variadic "any" argument in parser - should be array |
Дата | |
Msg-id | CAFj8pRDe_M-O7a06yZXgEk-LBVApFu2FrJgAkwCfPxNi8pYSig@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: checking variadic "any" argument in parser - should be array (Pavel Stehule <pavel.stehule@gmail.com>) |
Ответы |
Re: checking variadic "any" argument in parser - should be
array
Re: checking variadic "any" argument in parser - should be array Re: checking variadic "any" argument in parser - should be array |
Список | pgsql-hackers |
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.
Вложения
В списке pgsql-hackers по дате отправления: