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

Started by Pavel Stehulealmost 13 years ago15 messages

pavel.stehule@gmail.com

almost 13 years ago

1 attachment(s)

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

Attachments:

variadic_any_parser_check.patchapplication/octet-stream; name=variadic_any_parser_check.patchDownload

*** a/src/backend/catalog/pg_aggregate.c
--- b/src/backend/catalog/pg_aggregate.c
***************
*** 332,337 **** lookup_agg_function(List *fnName,
--- 332,338 ----
  	Oid			fnOid;
  	bool		retset;
  	int			nvargs;
+ 	Oid			vatype;
  	Oid		   *true_oid_array;
  	FuncDetailCode fdresult;
  	AclResult	aclresult;
***************
*** 346,352 **** lookup_agg_function(List *fnName,
  	 */
  	fdresult = func_get_detail(fnName, NIL, NIL,
  							   nargs, input_types, false, false,
! 							   &fnOid, rettype, &retset, &nvargs,
  							   &true_oid_array, NULL);
  
  	/* only valid case is a normal function not returning a set */
--- 347,354 ----
  	 */
  	fdresult = func_get_detail(fnName, NIL, NIL,
  							   nargs, input_types, false, false,
! 							   &fnOid, rettype, &retset,
! 							   &nvargs, &vatype,
  							   &true_oid_array, NULL);
  
  	/* only valid case is a normal function not returning a set */
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
***************
*** 79,84 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
--- 79,85 ----
  	Node	   *retval;
  	bool		retset;
  	int			nvargs;
+ 	Oid			vatype;
  	FuncDetailCode fdresult;
  
  	/*
***************
*** 214,220 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
  	fdresult = func_get_detail(funcname, fargs, argnames, nargs,
  							   actual_arg_types,
  							   !func_variadic, true,
! 							   &funcid, &rettype, &retset, &nvargs,
  							   &declared_arg_types, &argdefaults);
  	if (fdresult == FUNCDETAIL_COERCION)
  	{
--- 215,222 ----
  	fdresult = func_get_detail(funcname, fargs, argnames, nargs,
  							   actual_arg_types,
  							   !func_variadic, true,
! 							   &funcid, &rettype, &retset,
! 							   &nvargs, &vatype,
  							   &declared_arg_types, &argdefaults);
  	if (fdresult == FUNCDETAIL_COERCION)
  	{
***************
*** 376,381 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
--- 378,399 ----
  		fargs = lappend(fargs, newa);
  	}
  
+ 	/*
+ 	 * When function is called with VARIADIC labeled parameter,
+ 	 * and declared_arg_type is "ANY", then sanitize a real parameter
+ 	 * type now - should be an array.
+ 	 */
+ 	if (nargs > 0 && vatype == ANYOID && func_variadic)
+ 	{
+ 		Oid		va_arr_typid = actual_arg_types[nargs - 1];
+ 
+ 		if (!OidIsValid(get_element_type(va_arr_typid)))
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_DATATYPE_MISMATCH),
+ 					 errmsg("VARIADIC argument must be an array"),
+ 			  parser_errposition(pstate, exprLocation((Node *) llast(fargs)))));
+ 	}
+ 
  	/* build the appropriate output structure */
  	if (fdresult == FUNCDETAIL_NORMAL)
  	{
***************
*** 1015,1020 **** func_get_detail(List *funcname,
--- 1033,1039 ----
  				Oid *rettype,	/* return value */
  				bool *retset,	/* return value */
  				int *nvargs,	/* return value */
+ 				Oid *vatype,	/* return value */
  				Oid **true_typeids,		/* return value */
  				List **argdefaults)		/* optional return value */
  {
***************
*** 1233,1238 **** func_get_detail(List *funcname,
--- 1252,1258 ----
  		pform = (Form_pg_proc) GETSTRUCT(ftup);
  		*rettype = pform->prorettype;
  		*retset = pform->proretset;
+ 		*vatype = pform->provariadic;
  		/* fetch default args if caller wants 'em */
  		if (argdefaults && best_candidate->ndargs > 0)
  		{
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***************
*** 8550,8555 **** generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
--- 8550,8556 ----
  	Oid			p_rettype;
  	bool		p_retset;
  	int			p_nvargs;
+ 	Oid			p_vatype;
  	Oid		   *p_true_typeids;
  
  	proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
***************
*** 8600,8606 **** generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
  							   NIL, argnames, nargs, argtypes,
  							   !use_variadic, true,
  							   &p_funcid, &p_rettype,
! 							   &p_retset, &p_nvargs, &p_true_typeids, NULL);
  	if ((p_result == FUNCDETAIL_NORMAL ||
  		 p_result == FUNCDETAIL_AGGREGATE ||
  		 p_result == FUNCDETAIL_WINDOWFUNC) &&
--- 8601,8608 ----
  							   NIL, argnames, nargs, argtypes,
  							   !use_variadic, true,
  							   &p_funcid, &p_rettype,
! 							   &p_retset, &p_nvargs, &p_vatype,
! 							   &p_true_typeids, NULL);
  	if ((p_result == FUNCDETAIL_NORMAL ||
  		 p_result == FUNCDETAIL_AGGREGATE ||
  		 p_result == FUNCDETAIL_WINDOWFUNC) &&
*** a/src/backend/utils/adt/varlena.c
--- b/src/backend/utils/adt/varlena.c
***************
*** 3811,3817 **** concat_internal(const char *sepstr, int argidx,
  	 */
  	if (get_fn_expr_variadic(fcinfo->flinfo))
  	{
- 		Oid			arr_typid;
  		ArrayType  *arr;
  
  		/* Should have just the one argument */
--- 3811,3816 ----
***************
*** 3821,3841 **** concat_internal(const char *sepstr, int argidx,
  		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);
  
  		/*
***************
*** 4024,4030 **** text_format(PG_FUNCTION_ARGS)
  	/* If argument is marked VARIADIC, expand array into elements */
  	if (get_fn_expr_variadic(fcinfo->flinfo))
  	{
- 		Oid			arr_typid;
  		ArrayType  *arr;
  		int16		elmlen;
  		bool		elmbyval;
--- 4011,4016 ----
***************
*** 4039,4059 **** text_format(PG_FUNCTION_ARGS)
  			nitems = 0;
  		else
  		{
! 			/*
! 			 * 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, 1);
! 			if (!OidIsValid(arr_typid))
! 				elog(ERROR, "could not determine data type of format() 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(1);
  
  			/* Get info about array element type */
--- 4025,4033 ----
  			nitems = 0;
  		else
  		{
! 			/* Non-null argument had better be an array */
! 			Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo, 1))));
  
  			arr = PG_GETARG_ARRAYTYPE_P(1);
  
  			/* Get info about array element type */
*** a/src/include/parser/parse_func.h
--- b/src/include/parser/parse_func.h
***************
*** 53,60 **** extern FuncDetailCode func_get_detail(List *funcname,
  				int nargs, Oid *argtypes,
  				bool expand_variadic, bool expand_defaults,
  				Oid *funcid, Oid *rettype,
! 				bool *retset, int *nvargs, Oid **true_typeids,
! 				List **argdefaults);
  
  extern int func_match_argtypes(int nargs,
  					Oid *input_typeids,
--- 53,60 ----
  				int nargs, Oid *argtypes,
  				bool expand_variadic, bool expand_defaults,
  				Oid *funcid, Oid *rettype,
! 				bool *retset, int *nvargs, Oid *vatype,
! 				Oid **true_typeids, List **argdefaults);
  
  extern int func_match_argtypes(int nargs,
  					Oid *input_typeids,
*** a/src/test/regress/expected/text.out
--- b/src/test/regress/expected/text.out
***************
*** 149,161 **** select concat_ws(',', variadic array[1,2,3]);
   1,2,3
  (1 row)
  
! select concat_ws(',', variadic NULL);
   concat_ws 
  -----------
   
  (1 row)
  
! select concat(variadic NULL) is NULL;
   ?column? 
  ----------
   t
--- 149,161 ----
   1,2,3
  (1 row)
  
! select concat_ws(',', variadic NULL::int[]);
   concat_ws 
  -----------
   
  (1 row)
  
! select concat(variadic NULL::int[]) is NULL;
   ?column? 
  ----------
   t
***************
*** 170,175 **** select concat(variadic '{}'::int[]) = '';
--- 170,177 ----
  --should fail
  select concat_ws(',', variadic 10);
  ERROR:  VARIADIC argument must be an array
+ LINE 1: select concat_ws(',', variadic 10);
+                                        ^
  /*
   * format
   */
***************
*** 313,320 **** select format('%2$s, %1$s', variadic array[1, 2]);
   2, 1
  (1 row)
  
! -- variadic argument can be NULL, but should not be referenced
! select format('Hello', variadic NULL);
   format 
  --------
   Hello
--- 315,322 ----
   2, 1
  (1 row)
  
! -- variadic argument can be array type NULL, but should not be referenced
! select format('Hello', variadic NULL::int[]);
   format 
  --------
   Hello
*** a/src/test/regress/sql/text.sql
--- b/src/test/regress/sql/text.sql
***************
*** 47,54 **** select quote_literal(e'\\');
  -- check variadic labeled argument
  select concat(variadic array[1,2,3]);
  select concat_ws(',', variadic array[1,2,3]);
! select concat_ws(',', variadic NULL);
! select concat(variadic NULL) is NULL;
  select concat(variadic '{}'::int[]) = '';
  --should fail
  select concat_ws(',', variadic 10);
--- 47,54 ----
  -- check variadic labeled argument
  select concat(variadic array[1,2,3]);
  select concat_ws(',', variadic array[1,2,3]);
! select concat_ws(',', variadic NULL::int[]);
! select concat(variadic NULL::int[]) is NULL;
  select concat(variadic '{}'::int[]) = '';
  --should fail
  select concat_ws(',', variadic 10);
***************
*** 92,99 **** select format('%s, %s', variadic array[true, false]::text[]);
  -- check variadic with positional placeholders
  select format('%2$s, %1$s', variadic array['first', 'second']);
  select format('%2$s, %1$s', variadic array[1, 2]);
! -- variadic argument can be NULL, but should not be referenced
! select format('Hello', variadic NULL);
  -- variadic argument allows simulating more than FUNC_MAX_ARGS parameters
  select format(string_agg('%s',','), variadic array_agg(i))
  from generate_series(1,200) g(i);
--- 92,99 ----
  -- check variadic with positional placeholders
  select format('%2$s, %1$s', variadic array['first', 'second']);
  select format('%2$s, %1$s', variadic array[1, 2]);
! -- variadic argument can be array type NULL, but should not be referenced
! select format('Hello', variadic NULL::int[]);
  -- variadic argument allows simulating more than FUNC_MAX_ARGS parameters
  select format(string_agg('%s',','), variadic array_agg(i))
  from generate_series(1,200) g(i);

Jeevan Chalke

jeevan.chalke@enterprisedb.com

over 12 years ago

In reply to: Pavel Stehule (#1)

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

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.

Pavel Stehule

pavel.stehule@gmail.com

over 12 years ago

In reply to: Jeevan Chalke (#2)

1 attachment(s)

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

Hello

remastered version

Regards

Pavel

2013/6/26 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

Show quoted text

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.

Attachments:

variadic_any_parser_check-2.patchapplication/octet-stream; name=variadic_any_parser_check-2.patchDownload

*** a/src/backend/catalog/pg_aggregate.c
--- b/src/backend/catalog/pg_aggregate.c
***************
*** 332,337 **** lookup_agg_function(List *fnName,
--- 332,338 ----
  	Oid			fnOid;
  	bool		retset;
  	int			nvargs;
+ 	Oid			vatype;
  	Oid		   *true_oid_array;
  	FuncDetailCode fdresult;
  	AclResult	aclresult;
***************
*** 346,352 **** lookup_agg_function(List *fnName,
  	 */
  	fdresult = func_get_detail(fnName, NIL, NIL,
  							   nargs, input_types, false, false,
! 							   &fnOid, rettype, &retset, &nvargs,
  							   &true_oid_array, NULL);
  
  	/* only valid case is a normal function not returning a set */
--- 347,354 ----
  	 */
  	fdresult = func_get_detail(fnName, NIL, NIL,
  							   nargs, input_types, false, false,
! 							   &fnOid, rettype, &retset,
! 							   &nvargs, &vatype,
  							   &true_oid_array, NULL);
  
  	/* only valid case is a normal function not returning a set */
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
***************
*** 79,84 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
--- 79,85 ----
  	Node	   *retval;
  	bool		retset;
  	int			nvargs;
+ 	Oid			vatype;
  	FuncDetailCode fdresult;
  
  	/*
***************
*** 214,220 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
  	fdresult = func_get_detail(funcname, fargs, argnames, nargs,
  							   actual_arg_types,
  							   !func_variadic, true,
! 							   &funcid, &rettype, &retset, &nvargs,
  							   &declared_arg_types, &argdefaults);
  	if (fdresult == FUNCDETAIL_COERCION)
  	{
--- 215,222 ----
  	fdresult = func_get_detail(funcname, fargs, argnames, nargs,
  							   actual_arg_types,
  							   !func_variadic, true,
! 							   &funcid, &rettype, &retset,
! 							   &nvargs, &vatype,
  							   &declared_arg_types, &argdefaults);
  	if (fdresult == FUNCDETAIL_COERCION)
  	{
***************
*** 376,381 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
--- 378,399 ----
  		fargs = lappend(fargs, newa);
  	}
  
+ 	/*
+ 	 * When function is called with VARIADIC labeled parameter,
+ 	 * and declared_arg_type is "ANY", then sanitize a real parameter
+ 	 * type now - should be an array.
+ 	 */
+ 	if (nargs > 0 && vatype == ANYOID && func_variadic)
+ 	{
+ 		Oid		va_arr_typid = actual_arg_types[nargs - 1];
+ 
+ 		if (!OidIsValid(get_element_type(va_arr_typid)))
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_DATATYPE_MISMATCH),
+ 					 errmsg("VARIADIC argument must be an array"),
+ 			  parser_errposition(pstate, exprLocation((Node *) llast(fargs)))));
+ 	}
+ 
  	/* build the appropriate output structure */
  	if (fdresult == FUNCDETAIL_NORMAL)
  	{
***************
*** 1015,1020 **** func_get_detail(List *funcname,
--- 1033,1039 ----
  				Oid *rettype,	/* return value */
  				bool *retset,	/* return value */
  				int *nvargs,	/* return value */
+ 				Oid *vatype,	/* return value */
  				Oid **true_typeids,		/* return value */
  				List **argdefaults)		/* optional return value */
  {
***************
*** 1233,1238 **** func_get_detail(List *funcname,
--- 1252,1258 ----
  		pform = (Form_pg_proc) GETSTRUCT(ftup);
  		*rettype = pform->prorettype;
  		*retset = pform->proretset;
+ 		*vatype = pform->provariadic;
  		/* fetch default args if caller wants 'em */
  		if (argdefaults && best_candidate->ndargs > 0)
  		{
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***************
*** 8570,8575 **** generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
--- 8570,8576 ----
  	Oid			p_rettype;
  	bool		p_retset;
  	int			p_nvargs;
+ 	Oid			p_vatype;
  	Oid		   *p_true_typeids;
  
  	proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
***************
*** 8620,8626 **** generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
  							   NIL, argnames, nargs, argtypes,
  							   !use_variadic, true,
  							   &p_funcid, &p_rettype,
! 							   &p_retset, &p_nvargs, &p_true_typeids, NULL);
  	if ((p_result == FUNCDETAIL_NORMAL ||
  		 p_result == FUNCDETAIL_AGGREGATE ||
  		 p_result == FUNCDETAIL_WINDOWFUNC) &&
--- 8621,8628 ----
  							   NIL, argnames, nargs, argtypes,
  							   !use_variadic, true,
  							   &p_funcid, &p_rettype,
! 							   &p_retset, &p_nvargs, &p_vatype,
! 							   &p_true_typeids, NULL);
  	if ((p_result == FUNCDETAIL_NORMAL ||
  		 p_result == FUNCDETAIL_AGGREGATE ||
  		 p_result == FUNCDETAIL_WINDOWFUNC) &&
*** a/src/backend/utils/adt/varlena.c
--- b/src/backend/utils/adt/varlena.c
***************
*** 3820,3826 **** concat_internal(const char *sepstr, int argidx,
  	 */
  	if (get_fn_expr_variadic(fcinfo->flinfo))
  	{
- 		Oid			arr_typid;
  		ArrayType  *arr;
  
  		/* Should have just the one argument */
--- 3820,3825 ----
***************
*** 3830,3850 **** concat_internal(const char *sepstr, int argidx,
  		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);
  
  		/*
--- 3829,3837 ----
  		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);
  
  		/*
***************
*** 4049,4055 **** text_format(PG_FUNCTION_ARGS)
  	/* If argument is marked VARIADIC, expand array into elements */
  	if (get_fn_expr_variadic(fcinfo->flinfo))
  	{
- 		Oid			arr_typid;
  		ArrayType  *arr;
  		int16		elmlen;
  		bool		elmbyval;
--- 4036,4041 ----
***************
*** 4064,4084 **** text_format(PG_FUNCTION_ARGS)
  			nitems = 0;
  		else
  		{
! 			/*
! 			 * 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, 1);
! 			if (!OidIsValid(arr_typid))
! 				elog(ERROR, "could not determine data type of format() 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(1);
  
  			/* Get info about array element type */
--- 4050,4058 ----
  			nitems = 0;
  		else
  		{
! 			/* Non-null argument had better be an array */
! 			Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo, 1))));
  
  			arr = PG_GETARG_ARRAYTYPE_P(1);
  
  			/* Get info about array element type */
*** a/src/include/parser/parse_func.h
--- b/src/include/parser/parse_func.h
***************
*** 53,60 **** extern FuncDetailCode func_get_detail(List *funcname,
  				int nargs, Oid *argtypes,
  				bool expand_variadic, bool expand_defaults,
  				Oid *funcid, Oid *rettype,
! 				bool *retset, int *nvargs, Oid **true_typeids,
! 				List **argdefaults);
  
  extern int func_match_argtypes(int nargs,
  					Oid *input_typeids,
--- 53,60 ----
  				int nargs, Oid *argtypes,
  				bool expand_variadic, bool expand_defaults,
  				Oid *funcid, Oid *rettype,
! 				bool *retset, int *nvargs, Oid *vatype,
! 				Oid **true_typeids, List **argdefaults);
  
  extern int func_match_argtypes(int nargs,
  					Oid *input_typeids,
*** a/src/test/regress/expected/text.out
--- b/src/test/regress/expected/text.out
***************
*** 149,161 **** select concat_ws(',', variadic array[1,2,3]);
   1,2,3
  (1 row)
  
! select concat_ws(',', variadic NULL);
   concat_ws 
  -----------
   
  (1 row)
  
! select concat(variadic NULL) is NULL;
   ?column? 
  ----------
   t
--- 149,161 ----
   1,2,3
  (1 row)
  
! select concat_ws(',', variadic NULL::int[]);
   concat_ws 
  -----------
   
  (1 row)
  
! select concat(variadic NULL::int[]) is NULL;
   ?column? 
  ----------
   t
***************
*** 170,175 **** select concat(variadic '{}'::int[]) = '';
--- 170,177 ----
  --should fail
  select concat_ws(',', variadic 10);
  ERROR:  VARIADIC argument must be an array
+ LINE 1: select concat_ws(',', variadic 10);
+                                        ^
  /*
   * format
   */
***************
*** 315,322 **** select format('%2$s, %1$s', variadic array[1, 2]);
   2, 1
  (1 row)
  
! -- variadic argument can be NULL, but should not be referenced
! select format('Hello', variadic NULL);
   format 
  --------
   Hello
--- 317,324 ----
   2, 1
  (1 row)
  
! -- variadic argument can be array type NULL, but should not be referenced
! select format('Hello', variadic NULL::int[]);
   format 
  --------
   Hello
*** a/src/test/regress/sql/text.sql
--- b/src/test/regress/sql/text.sql
***************
*** 47,54 **** select quote_literal(e'\\');
  -- check variadic labeled argument
  select concat(variadic array[1,2,3]);
  select concat_ws(',', variadic array[1,2,3]);
! select concat_ws(',', variadic NULL);
! select concat(variadic NULL) is NULL;
  select concat(variadic '{}'::int[]) = '';
  --should fail
  select concat_ws(',', variadic 10);
--- 47,54 ----
  -- check variadic labeled argument
  select concat(variadic array[1,2,3]);
  select concat_ws(',', variadic array[1,2,3]);
! select concat_ws(',', variadic NULL::int[]);
! select concat(variadic NULL::int[]) is NULL;
  select concat(variadic '{}'::int[]) = '';
  --should fail
  select concat_ws(',', variadic 10);
***************
*** 93,100 **** select format('%s, %s', variadic array[true, false]::text[]);
  -- check variadic with positional placeholders
  select format('%2$s, %1$s', variadic array['first', 'second']);
  select format('%2$s, %1$s', variadic array[1, 2]);
! -- variadic argument can be NULL, but should not be referenced
! select format('Hello', variadic NULL);
  -- variadic argument allows simulating more than FUNC_MAX_ARGS parameters
  select format(string_agg('%s',','), variadic array_agg(i))
  from generate_series(1,200) g(i);
--- 93,100 ----
  -- check variadic with positional placeholders
  select format('%2$s, %1$s', variadic array['first', 'second']);
  select format('%2$s, %1$s', variadic array[1, 2]);
! -- variadic argument can be array type NULL, but should not be referenced
! select format('Hello', variadic NULL::int[]);
  -- variadic argument allows simulating more than FUNC_MAX_ARGS parameters
  select format(string_agg('%s',','), variadic array_agg(i))
  from generate_series(1,200) g(i);

Jeevan Chalke

jeevan.chalke@enterprisedb.com

over 12 years ago

In reply to: Pavel Stehule (#3)

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

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

Pavel Stehule

pavel.stehule@gmail.com

over 12 years ago

In reply to: Jeevan Chalke (#4)

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

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Jeevan Chalke

jeevan.chalke@enterprisedb.com

over 12 years ago

In reply to: Pavel Stehule (#5)

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

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

Pavel Stehule

pavel.stehule@gmail.com

over 12 years ago

In reply to: Jeevan Chalke (#6)

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

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Pavel Stehule

pavel.stehule@gmail.com

over 12 years ago

In reply to: Pavel Stehule (#7)

1 attachment(s)

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

Hello

updated patch - precious Assert, more comments

Regards

Pavel

2013/6/29 Pavel Stehule <pavel.stehule@gmail.com>:

Show quoted text

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.

Attachments:

variadic_any_parser_check-3.patchapplication/octet-stream; name=variadic_any_parser_check-3.patchDownload

*** a/src/backend/catalog/pg_aggregate.c
--- b/src/backend/catalog/pg_aggregate.c
***************
*** 332,337 **** lookup_agg_function(List *fnName,
--- 332,338 ----
  	Oid			fnOid;
  	bool		retset;
  	int			nvargs;
+ 	Oid			vatype;
  	Oid		   *true_oid_array;
  	FuncDetailCode fdresult;
  	AclResult	aclresult;
***************
*** 346,352 **** lookup_agg_function(List *fnName,
  	 */
  	fdresult = func_get_detail(fnName, NIL, NIL,
  							   nargs, input_types, false, false,
! 							   &fnOid, rettype, &retset, &nvargs,
  							   &true_oid_array, NULL);
  
  	/* only valid case is a normal function not returning a set */
--- 347,354 ----
  	 */
  	fdresult = func_get_detail(fnName, NIL, NIL,
  							   nargs, input_types, false, false,
! 							   &fnOid, rettype, &retset,
! 							   &nvargs, &vatype,
  							   &true_oid_array, NULL);
  
  	/* only valid case is a normal function not returning a set */
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
***************
*** 79,84 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
--- 79,85 ----
  	Node	   *retval;
  	bool		retset;
  	int			nvargs;
+ 	Oid			vatype;
  	FuncDetailCode fdresult;
  
  	/*
***************
*** 214,220 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
  	fdresult = func_get_detail(funcname, fargs, argnames, nargs,
  							   actual_arg_types,
  							   !func_variadic, true,
! 							   &funcid, &rettype, &retset, &nvargs,
  							   &declared_arg_types, &argdefaults);
  	if (fdresult == FUNCDETAIL_COERCION)
  	{
--- 215,222 ----
  	fdresult = func_get_detail(funcname, fargs, argnames, nargs,
  							   actual_arg_types,
  							   !func_variadic, true,
! 							   &funcid, &rettype, &retset,
! 							   &nvargs, &vatype,
  							   &declared_arg_types, &argdefaults);
  	if (fdresult == FUNCDETAIL_COERCION)
  	{
***************
*** 376,381 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
--- 378,399 ----
  		fargs = lappend(fargs, newa);
  	}
  
+ 	/*
+ 	 * When function is called with VARIADIC labeled parameter,
+ 	 * and declared_arg_type is "ANY", then sanitize a real parameter
+ 	 * type now - should be an array.
+ 	 */
+ 	if (nargs > 0 && vatype == ANYOID && func_variadic)
+ 	{
+ 		Oid		va_arr_typid = actual_arg_types[nargs - 1];
+ 
+ 		if (!OidIsValid(get_element_type(va_arr_typid)))
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_DATATYPE_MISMATCH),
+ 					 errmsg("VARIADIC argument must be an array"),
+ 			  parser_errposition(pstate, exprLocation((Node *) llast(fargs)))));
+ 	}
+ 
  	/* build the appropriate output structure */
  	if (fdresult == FUNCDETAIL_NORMAL)
  	{
***************
*** 1015,1020 **** func_get_detail(List *funcname,
--- 1033,1039 ----
  				Oid *rettype,	/* return value */
  				bool *retset,	/* return value */
  				int *nvargs,	/* return value */
+ 				Oid *vatype,	/* return value */
  				Oid **true_typeids,		/* return value */
  				List **argdefaults)		/* optional return value */
  {
***************
*** 1233,1238 **** func_get_detail(List *funcname,
--- 1252,1258 ----
  		pform = (Form_pg_proc) GETSTRUCT(ftup);
  		*rettype = pform->prorettype;
  		*retset = pform->proretset;
+ 		*vatype = pform->provariadic;
  		/* fetch default args if caller wants 'em */
  		if (argdefaults && best_candidate->ndargs > 0)
  		{
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***************
*** 8570,8575 **** generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
--- 8570,8576 ----
  	Oid			p_rettype;
  	bool		p_retset;
  	int			p_nvargs;
+ 	Oid			p_vatype;
  	Oid		   *p_true_typeids;
  
  	proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
***************
*** 8620,8626 **** generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
  							   NIL, argnames, nargs, argtypes,
  							   !use_variadic, true,
  							   &p_funcid, &p_rettype,
! 							   &p_retset, &p_nvargs, &p_true_typeids, NULL);
  	if ((p_result == FUNCDETAIL_NORMAL ||
  		 p_result == FUNCDETAIL_AGGREGATE ||
  		 p_result == FUNCDETAIL_WINDOWFUNC) &&
--- 8621,8628 ----
  							   NIL, argnames, nargs, argtypes,
  							   !use_variadic, true,
  							   &p_funcid, &p_rettype,
! 							   &p_retset, &p_nvargs, &p_vatype,
! 							   &p_true_typeids, NULL);
  	if ((p_result == FUNCDETAIL_NORMAL ||
  		 p_result == FUNCDETAIL_AGGREGATE ||
  		 p_result == FUNCDETAIL_WINDOWFUNC) &&
*** a/src/backend/utils/adt/varlena.c
--- b/src/backend/utils/adt/varlena.c
***************
*** 3820,3826 **** concat_internal(const char *sepstr, int argidx,
  	 */
  	if (get_fn_expr_variadic(fcinfo->flinfo))
  	{
- 		Oid			arr_typid;
  		ArrayType  *arr;
  
  		/* Should have just the one argument */
--- 3820,3825 ----
***************
*** 3831,3850 **** concat_internal(const char *sepstr, int 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);
  
  		/*
--- 3830,3845 ----
  			return NULL;
  
  		/*
! 		 * Non-null argument had better be an array
! 		 *
! 		 * Correct values are ensured by parser check, but this functionality
! 		 * can be called directly with bypassing a parser, so we should do
! 		 * some minimal check too - this form of call require correctly set
! 		 * expr argtype in flinfo.
  		 */
! 		Assert(OidIsValid(get_fn_expr_argtype(fcinfo->flinfo, argidx)));
! 		Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo, argidx))));
  
  		arr = PG_GETARG_ARRAYTYPE_P(argidx);
  
  		/*
***************
*** 4049,4055 **** text_format(PG_FUNCTION_ARGS)
  	/* If argument is marked VARIADIC, expand array into elements */
  	if (get_fn_expr_variadic(fcinfo->flinfo))
  	{
- 		Oid			arr_typid;
  		ArrayType  *arr;
  		int16		elmlen;
  		bool		elmbyval;
--- 4044,4049 ----
***************
*** 4065,4084 **** text_format(PG_FUNCTION_ARGS)
  		else
  		{
  			/*
! 			 * 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, 1);
! 			if (!OidIsValid(arr_typid))
! 				elog(ERROR, "could not determine data type of format() 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(1);
  
  			/* Get info about array element type */
--- 4059,4074 ----
  		else
  		{
  			/*
! 			 * Non-null argument had better be an array
! 			 *
! 			 * Correct values are ensured by parser check, but this functionality
! 			 * can be called directly with bypassing a parser, so we should do
! 			 * some minimal check too - this form of call require correctly set
! 			 * expr argtype in flinfo.
  			 */
! 			Assert(OidIsValid(get_fn_expr_argtype(fcinfo->flinfo, 1)));
! 			Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo, 1))));
  
  			arr = PG_GETARG_ARRAYTYPE_P(1);
  
  			/* Get info about array element type */
*** a/src/include/parser/parse_func.h
--- b/src/include/parser/parse_func.h
***************
*** 53,60 **** extern FuncDetailCode func_get_detail(List *funcname,
  				int nargs, Oid *argtypes,
  				bool expand_variadic, bool expand_defaults,
  				Oid *funcid, Oid *rettype,
! 				bool *retset, int *nvargs, Oid **true_typeids,
! 				List **argdefaults);
  
  extern int func_match_argtypes(int nargs,
  					Oid *input_typeids,
--- 53,60 ----
  				int nargs, Oid *argtypes,
  				bool expand_variadic, bool expand_defaults,
  				Oid *funcid, Oid *rettype,
! 				bool *retset, int *nvargs, Oid *vatype,
! 				Oid **true_typeids, List **argdefaults);
  
  extern int func_match_argtypes(int nargs,
  					Oid *input_typeids,
*** a/src/test/regress/expected/text.out
--- b/src/test/regress/expected/text.out
***************
*** 149,161 **** select concat_ws(',', variadic array[1,2,3]);
   1,2,3
  (1 row)
  
! select concat_ws(',', variadic NULL);
   concat_ws 
  -----------
   
  (1 row)
  
! select concat(variadic NULL) is NULL;
   ?column? 
  ----------
   t
--- 149,161 ----
   1,2,3
  (1 row)
  
! select concat_ws(',', variadic NULL::int[]);
   concat_ws 
  -----------
   
  (1 row)
  
! select concat(variadic NULL::int[]) is NULL;
   ?column? 
  ----------
   t
***************
*** 170,175 **** select concat(variadic '{}'::int[]) = '';
--- 170,177 ----
  --should fail
  select concat_ws(',', variadic 10);
  ERROR:  VARIADIC argument must be an array
+ LINE 1: select concat_ws(',', variadic 10);
+                                        ^
  /*
   * format
   */
***************
*** 315,322 **** select format('%2$s, %1$s', variadic array[1, 2]);
   2, 1
  (1 row)
  
! -- variadic argument can be NULL, but should not be referenced
! select format('Hello', variadic NULL);
   format 
  --------
   Hello
--- 317,324 ----
   2, 1
  (1 row)
  
! -- variadic argument can be array type NULL, but should not be referenced
! select format('Hello', variadic NULL::int[]);
   format 
  --------
   Hello
*** a/src/test/regress/sql/text.sql
--- b/src/test/regress/sql/text.sql
***************
*** 47,54 **** select quote_literal(e'\\');
  -- check variadic labeled argument
  select concat(variadic array[1,2,3]);
  select concat_ws(',', variadic array[1,2,3]);
! select concat_ws(',', variadic NULL);
! select concat(variadic NULL) is NULL;
  select concat(variadic '{}'::int[]) = '';
  --should fail
  select concat_ws(',', variadic 10);
--- 47,54 ----
  -- check variadic labeled argument
  select concat(variadic array[1,2,3]);
  select concat_ws(',', variadic array[1,2,3]);
! select concat_ws(',', variadic NULL::int[]);
! select concat(variadic NULL::int[]) is NULL;
  select concat(variadic '{}'::int[]) = '';
  --should fail
  select concat_ws(',', variadic 10);
***************
*** 93,100 **** select format('%s, %s', variadic array[true, false]::text[]);
  -- check variadic with positional placeholders
  select format('%2$s, %1$s', variadic array['first', 'second']);
  select format('%2$s, %1$s', variadic array[1, 2]);
! -- variadic argument can be NULL, but should not be referenced
! select format('Hello', variadic NULL);
  -- variadic argument allows simulating more than FUNC_MAX_ARGS parameters
  select format(string_agg('%s',','), variadic array_agg(i))
  from generate_series(1,200) g(i);
--- 93,100 ----
  -- check variadic with positional placeholders
  select format('%2$s, %1$s', variadic array['first', 'second']);
  select format('%2$s, %1$s', variadic array[1, 2]);
! -- variadic argument can be array type NULL, but should not be referenced
! select format('Hello', variadic NULL::int[]);
  -- variadic argument allows simulating more than FUNC_MAX_ARGS parameters
  select format(string_agg('%s',','), variadic array_agg(i))
  from generate_series(1,200) g(i);

Alvaro Herrera

alvherre@2ndquadrant.com

over 12 years ago

In reply to: Pavel Stehule (#8)

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10

Pavel Stehule

pavel.stehule@gmail.com

over 12 years ago

In reply to: Pavel Stehule (#8)

1 attachment(s)

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

2013/6/29 Pavel Stehule <pavel.stehule@gmail.com>:

Hello

updated patch - precious Assert, more comments

Regards

Pavel

stripped

Attachments:

variadic_any_parser_check-3.patchapplication/octet-stream; name=variadic_any_parser_check-3.patchDownload

*** a/src/backend/catalog/pg_aggregate.c
--- b/src/backend/catalog/pg_aggregate.c
***************
*** 332,337 **** lookup_agg_function(List *fnName,
--- 332,338 ----
  	Oid			fnOid;
  	bool		retset;
  	int			nvargs;
+ 	Oid			vatype;
  	Oid		   *true_oid_array;
  	FuncDetailCode fdresult;
  	AclResult	aclresult;
***************
*** 346,352 **** lookup_agg_function(List *fnName,
  	 */
  	fdresult = func_get_detail(fnName, NIL, NIL,
  							   nargs, input_types, false, false,
! 							   &fnOid, rettype, &retset, &nvargs,
  							   &true_oid_array, NULL);
  
  	/* only valid case is a normal function not returning a set */
--- 347,354 ----
  	 */
  	fdresult = func_get_detail(fnName, NIL, NIL,
  							   nargs, input_types, false, false,
! 							   &fnOid, rettype, &retset,
! 							   &nvargs, &vatype,
  							   &true_oid_array, NULL);
  
  	/* only valid case is a normal function not returning a set */
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
***************
*** 79,84 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
--- 79,85 ----
  	Node	   *retval;
  	bool		retset;
  	int			nvargs;
+ 	Oid			vatype;
  	FuncDetailCode fdresult;
  
  	/*
***************
*** 214,220 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
  	fdresult = func_get_detail(funcname, fargs, argnames, nargs,
  							   actual_arg_types,
  							   !func_variadic, true,
! 							   &funcid, &rettype, &retset, &nvargs,
  							   &declared_arg_types, &argdefaults);
  	if (fdresult == FUNCDETAIL_COERCION)
  	{
--- 215,222 ----
  	fdresult = func_get_detail(funcname, fargs, argnames, nargs,
  							   actual_arg_types,
  							   !func_variadic, true,
! 							   &funcid, &rettype, &retset,
! 							   &nvargs, &vatype,
  							   &declared_arg_types, &argdefaults);
  	if (fdresult == FUNCDETAIL_COERCION)
  	{
***************
*** 376,381 **** ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
--- 378,399 ----
  		fargs = lappend(fargs, newa);
  	}
  
+ 	/*
+ 	 * When function is called with VARIADIC labeled parameter,
+ 	 * and declared_arg_type is "ANY", then sanitize a real parameter
+ 	 * type now - should be an array.
+ 	 */
+ 	if (nargs > 0 && vatype == ANYOID && func_variadic)
+ 	{
+ 		Oid		va_arr_typid = actual_arg_types[nargs - 1];
+ 
+ 		if (!OidIsValid(get_element_type(va_arr_typid)))
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_DATATYPE_MISMATCH),
+ 					 errmsg("VARIADIC argument must be an array"),
+ 			  parser_errposition(pstate, exprLocation((Node *) llast(fargs)))));
+ 	}
+ 
  	/* build the appropriate output structure */
  	if (fdresult == FUNCDETAIL_NORMAL)
  	{
***************
*** 1015,1020 **** func_get_detail(List *funcname,
--- 1033,1039 ----
  				Oid *rettype,	/* return value */
  				bool *retset,	/* return value */
  				int *nvargs,	/* return value */
+ 				Oid *vatype,	/* return value */
  				Oid **true_typeids,		/* return value */
  				List **argdefaults)		/* optional return value */
  {
***************
*** 1233,1238 **** func_get_detail(List *funcname,
--- 1252,1258 ----
  		pform = (Form_pg_proc) GETSTRUCT(ftup);
  		*rettype = pform->prorettype;
  		*retset = pform->proretset;
+ 		*vatype = pform->provariadic;
  		/* fetch default args if caller wants 'em */
  		if (argdefaults && best_candidate->ndargs > 0)
  		{
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***************
*** 8570,8575 **** generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
--- 8570,8576 ----
  	Oid			p_rettype;
  	bool		p_retset;
  	int			p_nvargs;
+ 	Oid			p_vatype;
  	Oid		   *p_true_typeids;
  
  	proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
***************
*** 8620,8626 **** generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
  							   NIL, argnames, nargs, argtypes,
  							   !use_variadic, true,
  							   &p_funcid, &p_rettype,
! 							   &p_retset, &p_nvargs, &p_true_typeids, NULL);
  	if ((p_result == FUNCDETAIL_NORMAL ||
  		 p_result == FUNCDETAIL_AGGREGATE ||
  		 p_result == FUNCDETAIL_WINDOWFUNC) &&
--- 8621,8628 ----
  							   NIL, argnames, nargs, argtypes,
  							   !use_variadic, true,
  							   &p_funcid, &p_rettype,
! 							   &p_retset, &p_nvargs, &p_vatype,
! 							   &p_true_typeids, NULL);
  	if ((p_result == FUNCDETAIL_NORMAL ||
  		 p_result == FUNCDETAIL_AGGREGATE ||
  		 p_result == FUNCDETAIL_WINDOWFUNC) &&
*** a/src/backend/utils/adt/varlena.c
--- b/src/backend/utils/adt/varlena.c
***************
*** 3820,3826 **** concat_internal(const char *sepstr, int argidx,
  	 */
  	if (get_fn_expr_variadic(fcinfo->flinfo))
  	{
- 		Oid			arr_typid;
  		ArrayType  *arr;
  
  		/* Should have just the one argument */
--- 3820,3825 ----
***************
*** 3831,3850 **** concat_internal(const char *sepstr, int 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);
  
  		/*
--- 3830,3845 ----
  			return NULL;
  
  		/*
! 		 * Non-null argument had better be an array
! 		 *
! 		 * Correct values are ensured by parser check, but this functionality
! 		 * can be called directly with bypassing a parser, so we should do
! 		 * some minimal check too - this form of call require correctly set
! 		 * expr argtype in flinfo.
  		 */
! 		Assert(OidIsValid(get_fn_expr_argtype(fcinfo->flinfo, argidx)));
! 		Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo, argidx))));
  
  		arr = PG_GETARG_ARRAYTYPE_P(argidx);
  
  		/*
***************
*** 4049,4055 **** text_format(PG_FUNCTION_ARGS)
  	/* If argument is marked VARIADIC, expand array into elements */
  	if (get_fn_expr_variadic(fcinfo->flinfo))
  	{
- 		Oid			arr_typid;
  		ArrayType  *arr;
  		int16		elmlen;
  		bool		elmbyval;
--- 4044,4049 ----
***************
*** 4065,4084 **** text_format(PG_FUNCTION_ARGS)
  		else
  		{
  			/*
! 			 * 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, 1);
! 			if (!OidIsValid(arr_typid))
! 				elog(ERROR, "could not determine data type of format() 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(1);
  
  			/* Get info about array element type */
--- 4059,4074 ----
  		else
  		{
  			/*
! 			 * Non-null argument had better be an array
! 			 *
! 			 * Correct values are ensured by parser check, but this functionality
! 			 * can be called directly with bypassing a parser, so we should do
! 			 * some minimal check too - this form of call require correctly set
! 			 * expr argtype in flinfo.
  			 */
! 			Assert(OidIsValid(get_fn_expr_argtype(fcinfo->flinfo, 1)));
! 			Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo->flinfo, 1))));
  
  			arr = PG_GETARG_ARRAYTYPE_P(1);
  
  			/* Get info about array element type */
*** a/src/include/parser/parse_func.h
--- b/src/include/parser/parse_func.h
***************
*** 53,60 **** extern FuncDetailCode func_get_detail(List *funcname,
  				int nargs, Oid *argtypes,
  				bool expand_variadic, bool expand_defaults,
  				Oid *funcid, Oid *rettype,
! 				bool *retset, int *nvargs, Oid **true_typeids,
! 				List **argdefaults);
  
  extern int func_match_argtypes(int nargs,
  					Oid *input_typeids,
--- 53,60 ----
  				int nargs, Oid *argtypes,
  				bool expand_variadic, bool expand_defaults,
  				Oid *funcid, Oid *rettype,
! 				bool *retset, int *nvargs, Oid *vatype,
! 				Oid **true_typeids, List **argdefaults);
  
  extern int func_match_argtypes(int nargs,
  					Oid *input_typeids,
*** a/src/test/regress/expected/text.out
--- b/src/test/regress/expected/text.out
***************
*** 149,161 **** select concat_ws(',', variadic array[1,2,3]);
   1,2,3
  (1 row)
  
! select concat_ws(',', variadic NULL);
   concat_ws 
  -----------
   
  (1 row)
  
! select concat(variadic NULL) is NULL;
   ?column? 
  ----------
   t
--- 149,161 ----
   1,2,3
  (1 row)
  
! select concat_ws(',', variadic NULL::int[]);
   concat_ws 
  -----------
   
  (1 row)
  
! select concat(variadic NULL::int[]) is NULL;
   ?column? 
  ----------
   t
***************
*** 170,175 **** select concat(variadic '{}'::int[]) = '';
--- 170,177 ----
  --should fail
  select concat_ws(',', variadic 10);
  ERROR:  VARIADIC argument must be an array
+ LINE 1: select concat_ws(',', variadic 10);
+                                        ^
  /*
   * format
   */
***************
*** 315,322 **** select format('%2$s, %1$s', variadic array[1, 2]);
   2, 1
  (1 row)
  
! -- variadic argument can be NULL, but should not be referenced
! select format('Hello', variadic NULL);
   format 
  --------
   Hello
--- 317,324 ----
   2, 1
  (1 row)
  
! -- variadic argument can be array type NULL, but should not be referenced
! select format('Hello', variadic NULL::int[]);
   format 
  --------
   Hello
*** a/src/test/regress/sql/text.sql
--- b/src/test/regress/sql/text.sql
***************
*** 47,54 **** select quote_literal(e'\\');
  -- check variadic labeled argument
  select concat(variadic array[1,2,3]);
  select concat_ws(',', variadic array[1,2,3]);
! select concat_ws(',', variadic NULL);
! select concat(variadic NULL) is NULL;
  select concat(variadic '{}'::int[]) = '';
  --should fail
  select concat_ws(',', variadic 10);
--- 47,54 ----
  -- check variadic labeled argument
  select concat(variadic array[1,2,3]);
  select concat_ws(',', variadic array[1,2,3]);
! select concat_ws(',', variadic NULL::int[]);
! select concat(variadic NULL::int[]) is NULL;
  select concat(variadic '{}'::int[]) = '';
  --should fail
  select concat_ws(',', variadic 10);
***************
*** 93,100 **** select format('%s, %s', variadic array[true, false]::text[]);
  -- check variadic with positional placeholders
  select format('%2$s, %1$s', variadic array['first', 'second']);
  select format('%2$s, %1$s', variadic array[1, 2]);
! -- variadic argument can be NULL, but should not be referenced
! select format('Hello', variadic NULL);
  -- variadic argument allows simulating more than FUNC_MAX_ARGS parameters
  select format(string_agg('%s',','), variadic array_agg(i))
  from generate_series(1,200) g(i);
--- 93,100 ----
  -- check variadic with positional placeholders
  select format('%2$s, %1$s', variadic array['first', 'second']);
  select format('%2$s, %1$s', variadic array[1, 2]);
! -- variadic argument can be array type NULL, but should not be referenced
! select format('Hello', variadic NULL::int[]);
  -- variadic argument allows simulating more than FUNC_MAX_ARGS parameters
  select format(string_agg('%s',','), variadic array_agg(i))
  from generate_series(1,200) g(i);

#11

Jeevan Chalke

jeevan.chalke@enterprisedb.com

over 12 years ago

In reply to: Pavel Stehule (#10)

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

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

Andrew Dunstan

andrew@dunslane.net

over 12 years ago

In reply to: Pavel Stehule (#8)

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13

Pavel Stehule

pavel.stehule@gmail.com

over 12 years ago

In reply to: Andrew Dunstan (#12)

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#14

Andrew Dunstan

andrew@dunslane.net

over 12 years ago

In reply to: Pavel Stehule (#13)

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#15

Pavel Stehule

pavel.stehule@gmail.com

over 12 years ago

In reply to: Andrew Dunstan (#14)

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers