Thread

1. checking variadic "any" argument in parser - should be array

   Pavel Stehule <pavel.stehule@gmail.com> — 2013-01-26T03:52:41Z

   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
   
   Regards
   
   Pavel
   

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

   Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2013-06-26T11:12:42Z

   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.
   

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

   Pavel Stehule <pavel.stehule@gmail.com> — 2013-06-26T19:05:09Z

   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.
   

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

   Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2013-06-27T09:29:01Z

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

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

   Pavel Stehule <pavel.stehule@gmail.com> — 2013-06-27T21:23:52Z

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

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

   Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2013-06-28T07:32:17Z

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

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

   Pavel Stehule <pavel.stehule@gmail.com> — 2013-06-29T09:19:43Z

   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.
   
   
   

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

   Pavel Stehule <pavel.stehule@gmail.com> — 2013-06-29T19:29:23Z

   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.
   

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

   Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-07-01T15:00:39Z

   Pavel Stehule escribió:
   > Hello
   > 
   > updated patch - precious Assert, more comments
   
   Pavel, can you please remove quoted text from messages you reply to?
   This message has 10kb of completely useless text.
   
   Thanks,
   
   -- 
   Álvaro Herrera                http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2013-07-01T15:06:13Z

    2013/6/29 Pavel Stehule <pavel.stehule@gmail.com>:
    > Hello
    >
    > updated patch - precious Assert, more comments
    >
    > Regards
    >
    > Pavel
    >
    
    stripped
    

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

    Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2013-07-02T09:48:31Z

    On Mon, Jul 1, 2013 at 8:36 PM, Pavel Stehule <pavel.stehule@gmail.com>wrote:
    
    > 2013/6/29 Pavel Stehule <pavel.stehule@gmail.com>:
    > > Hello
    > >
    > > updated patch - precious Assert, more comments
    > >
    > > Regards
    > >
    > > Pavel
    > >
    >
    > stripped
    >
    
    Thanks.
    
    Patch looks good to me now.
    Revalidated and didn't see any issue so marking "Ready For Committer".
    
    Thanks Pavel.
    
    -- 
    Jeevan B Chalke
    Senior Software Engineer, R&D
    EnterpriseDB Corporation
    The Enterprise PostgreSQL Company
    

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

    Andrew Dunstan <andrew@dunslane.net> — 2013-07-13T22:02:50Z

    On 06/29/2013 03:29 PM, Pavel Stehule wrote:
    
    
    
    >>>>> 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.
    
    
    Is the behaviour change really unavoidable? Is it really what we want? 
    Nobody seems to have picked up on this except the author and the 
    reviewer. I'd hate us to do this and then surprise people. I'm not sure 
    how many people are using VARIADIC "any", but I have started doing so 
    and expect to do so more, and I suspect I'm not alone.
    
    cheers
    
    andrew
    
    
    
    

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2013-07-14T04:28:34Z

    Hello
    
    2013/7/14 Andrew Dunstan <andrew@dunslane.net>:
    >
    > On 06/29/2013 03:29 PM, Pavel Stehule wrote:
    >
    >
    >
    >>>>>> 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.
    >
    >
    >
    > Is the behaviour change really unavoidable? Is it really what we want?
    > Nobody seems to have picked up on this except the author and the reviewer.
    > I'd hate us to do this and then surprise people. I'm not sure how many
    > people are using VARIADIC "any", but I have started doing so and expect to
    > do so more, and I suspect I'm not alone.
    
    It doesn't disallow NULL - it disallow nonarray types on this
    possition, because there are must be only array type values. Other
    possible usage created unambiguous behave.
    
    so SELECT varfx(VARIADIC NULL) -- is disallowed
    but SELECT varfx(VARIADIC NULL::text[]) -- is allowed
    
    for example, I can wrote SELECT varfx(10,20,30), but I cannot write
    SELECT varfx(VARIADIC 10,20,30) - because this behave should be
    undefined.
    
    Can me  send, your use case, where this check is unwanted, please.
    
    The execution of variadic function can be little bit faster, because
    this check is moved from execution to parser stage (and it is reason,
    why I cannot to check NULL, because I have no simply access to
    information about some parameter is constant or not.
    
    It should to fix unwanted behave where VARIADIC keyword was ignored -
    I am sure so this is some what we want, but I don't know your
    arguments against, so please, send me you use case.
    
    Regards
    
    Pavel
    
    
    
    >
    > cheers
    >
    > andrew
    >
    
    
    

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

    Andrew Dunstan <andrew@dunslane.net> — 2013-07-15T20:43:35Z

    On 07/14/2013 12:28 AM, Pavel Stehule wrote:
    > Hello
    >
    > 2013/7/14 Andrew Dunstan <andrew@dunslane.net>:
    >> On 06/29/2013 03:29 PM, Pavel Stehule wrote:
    >>
    >>
    >>
    >>>>>>> 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.
    >>
    >>
    >> Is the behaviour change really unavoidable? Is it really what we want?
    >> Nobody seems to have picked up on this except the author and the reviewer.
    >> I'd hate us to do this and then surprise people. I'm not sure how many
    >> people are using VARIADIC "any", but I have started doing so and expect to
    >> do so more, and I suspect I'm not alone.
    > It doesn't disallow NULL - it disallow nonarray types on this
    > possition, because there are must be only array type values. Other
    > possible usage created unambiguous behave.
    >
    > so SELECT varfx(VARIADIC NULL) -- is disallowed
    > but SELECT varfx(VARIADIC NULL::text[]) -- is allowed
    
    
    Quite so, I understand exactly what the defined behaviour will be.
    
    
    >
    > for example, I can wrote SELECT varfx(10,20,30), but I cannot write
    > SELECT varfx(VARIADIC 10,20,30) - because this behave should be
    > undefined.
    >
    > Can me  send, your use case, where this check is unwanted, please.
    
    
    The only question I raised was for the NULL case. If you're not saying 
    "VARIADIC NULL" then I have no issue.
    
    Anyway, nobody else seem to care much (and I suspect very few people are 
    writing VARIADIC "any" functions anyway, apart from you and me). So I'll 
    see about getting this committed shortly.
    
    cheers
    
    andrew
    
    
    
    

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2013-07-16T16:41:25Z

    2013/7/15 Andrew Dunstan <andrew@dunslane.net>:
    >
    > On 07/14/2013 12:28 AM, Pavel Stehule wrote:
    >>
    >> Hello
    >>
    >> 2013/7/14 Andrew Dunstan <andrew@dunslane.net>:
    >>>
    >>> On 06/29/2013 03:29 PM, Pavel Stehule wrote:
    >>>
    >>>
    >>>
    >>>>>>>> 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.
    >>>
    >>>
    >>>
    >>> Is the behaviour change really unavoidable? Is it really what we want?
    >>> Nobody seems to have picked up on this except the author and the
    >>> reviewer.
    >>> I'd hate us to do this and then surprise people. I'm not sure how many
    >>> people are using VARIADIC "any", but I have started doing so and expect
    >>> to
    >>> do so more, and I suspect I'm not alone.
    >>
    >> It doesn't disallow NULL - it disallow nonarray types on this
    >> possition, because there are must be only array type values. Other
    >> possible usage created unambiguous behave.
    >>
    >> so SELECT varfx(VARIADIC NULL) -- is disallowed
    >> but SELECT varfx(VARIADIC NULL::text[]) -- is allowed
    >
    >
    >
    > Quite so, I understand exactly what the defined behaviour will be.
    >
    >
    >
    >>
    >> for example, I can wrote SELECT varfx(10,20,30), but I cannot write
    >> SELECT varfx(VARIADIC 10,20,30) - because this behave should be
    >> undefined.
    >>
    >> Can me  send, your use case, where this check is unwanted, please.
    >
    >
    >
    > The only question I raised was for the NULL case. If you're not saying
    > "VARIADIC NULL" then I have no issue.
    
    NULL is allowed - but it should be typed.
    
    >
    > Anyway, nobody else seem to care much (and I suspect very few people are
    > writing VARIADIC "any" functions anyway, apart from you and me). So I'll see
    > about getting this committed shortly.
    >
    
    exactly
    
    Regards
    
    Pavel
    
    > cheers
    >
    > andrew
    >