test_plpgsql_array_cleanup.patch

*** a/src/pl/plpgsql/src/gram.y
--- b/src/pl/plpgsql/src/gram.y
***************
*** 192,198 **** static	List			*read_raise_options(void);
  %type <stmt>	stmt_return stmt_raise stmt_execsql
  %type <stmt>	stmt_dynexecute stmt_for stmt_perform stmt_getdiag
  %type <stmt>	stmt_open stmt_fetch stmt_move stmt_close stmt_null
! %type <stmt>	stmt_case stmt_foreach_a 
  
  %type <list>	proc_exceptions
  %type <exception_block> exception_sect
--- 192,198 ----
  %type <stmt>	stmt_return stmt_raise stmt_execsql
  %type <stmt>	stmt_dynexecute stmt_for stmt_perform stmt_getdiag
  %type <stmt>	stmt_open stmt_fetch stmt_move stmt_close stmt_null
! %type <stmt>	stmt_case stmt_foreach_a
  
  %type <list>	proc_exceptions
  %type <exception_block> exception_sect
***************
*** 1392,1398 **** for_variable	: T_DATUM
  					}
  				;
  
! stmt_foreach_a		: opt_block_label K_FOREACH foreach_control K_IN  expr_until_loop loop_body
  					{
  						PLpgSQL_stmt_foreach_a *new = palloc0(sizeof(PLpgSQL_stmt_foreach_a));
  						new->cmd_type = PLPGSQL_STMT_FOREACH_A;
--- 1392,1398 ----
  					}
  				;
  
! stmt_foreach_a		: opt_block_label K_FOREACH foreach_control K_IN expr_until_loop loop_body
  					{
  						PLpgSQL_stmt_foreach_a *new = palloc0(sizeof(PLpgSQL_stmt_foreach_a));
  						new->cmd_type = PLPGSQL_STMT_FOREACH_A;
***************
*** 1423,1429 **** stmt_foreach_a		: opt_block_label K_FOREACH foreach_control K_IN  expr_until_loo
  						{
  							ereport(ERROR,
  									(errcode(ERRCODE_SYNTAX_ERROR),
! 									 errmsg("loop variable of loop over arrat must be a record, row variable, scalar variable or list of scalar variables"),
  											 parser_errposition(@3)));
  						}
  
--- 1423,1429 ----
  						{
  							ereport(ERROR,
  									(errcode(ERRCODE_SYNTAX_ERROR),
! 									 errmsg("loop variable of loop over array must be a record, row variable, scalar variable or list of scalar variables"),
  											 parser_errposition(@3)));
  						}
  
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
***************
*** 2035,2083 **** exec_stmt_forc(PLpgSQL_execstate *estate, PLpgSQL_stmt_forc *stmt)
  
  /* ----------
   * exec_stmt_foreach_a			Implements loop over array
-  *
   * ----------
   */
! static int 
  exec_stmt_foreach_a(PLpgSQL_execstate *estate, PLpgSQL_stmt_foreach_a *stmt)
  {
  	Datum		value;
  	bool		isnull;
  	Oid			valtype;
! 	int	numelems = 0;
! 	Oid 	array_typelem;
! 	int	idx;
! 	ArrayType	*arr;
! 	char *ptr;
! 	bits8	*arraynullsptr;
! 	int16	elmlen;
! 	bool	elmbyval;
! 	char	elmalign;
  	PLpgSQL_datum *ctrl_var;
  	bool		found = false;
  	int			rc = PLPGSQL_RC_OK;
! 	int		nitems = 1;
  
! 	/* get a result of array_expr */
  	value = exec_eval_expr(estate, stmt->expr, &isnull, &valtype);
  	if (isnull)
  		ereport(ERROR,
  				(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
  				 errmsg("NULL value isn't allowed as parameter of FOREACH-IN")));
  
! 	/* check a result of expression - must be a array */
  	array_typelem = get_element_type(valtype);
  
  	if (!OidIsValid(array_typelem))
  		ereport(ERROR,
  				(errcode(ERRCODE_DATATYPE_MISMATCH),
! 				 errmsg("result of expression isn't array"),
! 				 errdetail("result of expression is %s", 
  									format_type_be(valtype))));
!  
! 	/* copy a result and takes some infos */
  	arr = DatumGetArrayTypePCopy(value);
  	numelems = ArrayGetNItems(ARR_NDIM(arr), ARR_DIMS(arr));
  	ptr = ARR_DATA_PTR(arr);
  	arraynullsptr = ARR_NULLBITMAP(arr);
  	get_typlenbyvalalign(ARR_ELEMTYPE(arr),
--- 2035,2086 ----
  
  /* ----------
   * exec_stmt_foreach_a			Implements loop over array
   * ----------
   */
! static int
  exec_stmt_foreach_a(PLpgSQL_execstate *estate, PLpgSQL_stmt_foreach_a *stmt)
  {
  	Datum		value;
  	bool		isnull;
  	Oid			valtype;
! 	int			numelems = 0;
! 	Oid			array_typelem;
! 	int			idx;
! 	ArrayType  *arr;
! 	char	   *ptr;
! 	bits8	   *arraynullsptr;
! 	int16		elmlen;
! 	bool		elmbyval;
! 	char		elmalign;
  	PLpgSQL_datum *ctrl_var;
  	bool		found = false;
  	int			rc = PLPGSQL_RC_OK;
! 	int			nitems = 1;
  
! 	/* get the value of the array expression using array_expr */
  	value = exec_eval_expr(estate, stmt->expr, &isnull, &valtype);
  	if (isnull)
  		ereport(ERROR,
  				(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
  				 errmsg("NULL value isn't allowed as parameter of FOREACH-IN")));
  
! 	/* check the type of the expression - must be an array */
  	array_typelem = get_element_type(valtype);
  
  	if (!OidIsValid(array_typelem))
  		ereport(ERROR,
  				(errcode(ERRCODE_DATATYPE_MISMATCH),
! 				 errmsg("result of expression isn't an array"),
! 				 errdetail("result of expression is %s",
  									format_type_be(valtype))));
! 
! 	/* make a copy of the result */
  	arr = DatumGetArrayTypePCopy(value);
+ 
+ 	/* Calculate the number of elements we're going to loop through */
  	numelems = ArrayGetNItems(ARR_NDIM(arr), ARR_DIMS(arr));
+ 
+ 	/* Gather information about the array */
  	ptr = ARR_DATA_PTR(arr);
  	arraynullsptr = ARR_NULLBITMAP(arr);
  	get_typlenbyvalalign(ARR_ELEMTYPE(arr),
***************
*** 2085,2101 **** exec_stmt_foreach_a(PLpgSQL_execstate *estate, PLpgSQL_stmt_foreach_a *stmt)
  						&elmbyval,
  						&elmalign);
  
! 	/* clean a stack */
  	exec_eval_cleanup(estate);
  
  	if (stmt->slice > 0)
  	{
  		if (stmt->rec != NULL || stmt->row != NULL)
  			ereport(ERROR,
  					(errcode(ERRCODE_DATATYPE_MISMATCH),
! 					 errmsg("target variable \"%s\" isn't a of array type",
  							    stmt->row ? stmt->row->refname : stmt->rec->refname),
! 					 errhint("Assigned value will be a value of array type.")));
  
  		if (stmt->slice > ARR_NDIM(arr))
  			ereport(ERROR,
--- 2088,2112 ----
  						&elmbyval,
  						&elmalign);
  
! 	/* Clean up any leftover temporary memory */
  	exec_eval_cleanup(estate);
  
+ 	/*
+ 	 * If the target needs to be an array, check that it actually is,
+ 	 * and that it has a valid dimension for the array we're looping
+ 	 * through.
+ 	 *
+ 	 * XXX: Is this redundant?  Could it be merged with the below
+ 	 * set of conditionals?
+ 	 */
  	if (stmt->slice > 0)
  	{
  		if (stmt->rec != NULL || stmt->row != NULL)
  			ereport(ERROR,
  					(errcode(ERRCODE_DATATYPE_MISMATCH),
! 					 errmsg("target variable \"%s\" isn't an array type",
  							    stmt->row ? stmt->row->refname : stmt->rec->refname),
! 					 errhint("Value assigned will be of an array type.")));
  
  		if (stmt->slice > ARR_NDIM(arr))
  			ereport(ERROR,
***************
*** 2104,2110 **** exec_stmt_foreach_a(PLpgSQL_execstate *estate, PLpgSQL_stmt_foreach_a *stmt)
  								    stmt->slice)));
  	}
  
! 	/* get a target variable */
  	if (stmt->var != NULL)
  	{
  		int	typoid = stmt->var->datatype->typoid;
--- 2115,2121 ----
  								    stmt->slice)));
  	}
  
! 	/* Set up the target variable */
  	if (stmt->var != NULL)
  	{
  		int	typoid = stmt->var->datatype->typoid;
***************
*** 2112,2123 **** exec_stmt_foreach_a(PLpgSQL_execstate *estate, PLpgSQL_stmt_foreach_a *stmt)
  
  		if (stmt->slice > 0)
  		{
! 			/* target variable have to be a array type */
  			if (elmoid == InvalidOid)
  				ereport(ERROR,
  						(errcode(ERRCODE_DATATYPE_MISMATCH),
! 						 errmsg("target variable \"%s\" for sliced array should be a array",
  										stmt->var->refname)));
  			nitems = ArrayGetNItems(stmt->slice, ARR_DIMS(arr) + ARR_NDIM(arr) - stmt->slice);
  		}
  		else
--- 2123,2136 ----
  
  		if (stmt->slice > 0)
  		{
! 			/* target variable has to be an array */
  			if (elmoid == InvalidOid)
  				ereport(ERROR,
  						(errcode(ERRCODE_DATATYPE_MISMATCH),
! 						 errmsg("target variable \"%s\" for sliced array should be an array type",
  										stmt->var->refname)));
+ 
+ 			/* Determine the number of items in the slice */
  			nitems = ArrayGetNItems(stmt->slice, ARR_DIMS(arr) + ARR_NDIM(arr) - stmt->slice);
  		}
  		else
***************
*** 2126,2133 **** exec_stmt_foreach_a(PLpgSQL_execstate *estate, PLpgSQL_stmt_foreach_a *stmt)
  			if (elmoid != InvalidOid)
  				ereport(ERROR,
  						(errcode(ERRCODE_DATATYPE_MISMATCH),
! 						 errmsg("target variable \"%s\" is a array", 
! 										stmt->var->refname)));
  		}
  		ctrl_var = estate->datums[stmt->var->dno];
  	}
--- 2139,2147 ----
  			if (elmoid != InvalidOid)
  				ereport(ERROR,
  						(errcode(ERRCODE_DATATYPE_MISMATCH),
! 						 errmsg("target variable \"%s\" is an array",
! 										stmt->var->refname),
! 						 errhint("Value assigned will be of a scalar type")));
  		}
  		ctrl_var = estate->datums[stmt->var->dno];
  	}
***************
*** 2135,2147 **** exec_stmt_foreach_a(PLpgSQL_execstate *estate, PLpgSQL_stmt_foreach_a *stmt)
  	{
  		ctrl_var = estate->datums[stmt->row->dno];
  	}
! 	else 
  	{
  		ctrl_var = estate->datums[stmt->rec->dno];
  	}
  
  	/*
  	 * Now do the loop
  	 */
  	idx = 0;
  	while (idx < numelems)
--- 2149,2164 ----
  	{
  		ctrl_var = estate->datums[stmt->row->dno];
  	}
! 	else
  	{
  		ctrl_var = estate->datums[stmt->rec->dno];
  	}
  
  	/*
  	 * Now do the loop
+ 	 *
+ 	 * XXX: These variable names are.. unfortunate.  Could we come up
+ 	 * with soe which are more descriptive?
  	 */
  	idx = 0;
  	while (idx < numelems)
***************
*** 2151,2158 **** exec_stmt_foreach_a(PLpgSQL_execstate *estate, PLpgSQL_stmt_foreach_a *stmt)
--- 2168,2180 ----
  
  		found = true;				/* looped at least once */
  
+ 		/* Loop through the items in this slice to build up the
+ 		 * array slice to be used through the exec statements ... ?
+ 		 * XXX: This really needs some better commenting.. */
  		for (j = 0; j < nitems; j++)
  		{
+ 			/* XXX: Do we have a better way to check arraynullsptr?
+ 			 * Maybe a macro of some kind ..? */
  			if (arraynullsptr != NULL && !(arraynullsptr[idx / 8] & (1 << (idx % 8))))
  			{
  				isnull = true;
***************
*** 2168,2193 **** exec_stmt_foreach_a(PLpgSQL_execstate *estate, PLpgSQL_stmt_foreach_a *stmt)
  			}
  
  			if (stmt->slice > 0)
! 				astate = accumArrayResult(astate, value, isnull, 
! 									array_typelem, 
  									CurrentMemoryContext);
  			idx++;
  		}
  
! 		/* 
! 		 * store a item to variable - we expecting so almost all
! 		 * iteration will be over scalar values, so it is reason
! 		 * why we don't create a fake tuple over scalar and we 
! 		 * don't use a exec_move_row for scalars. This is about 
! 		 * four times faster.
  		 */
  		if (astate != NULL)
  		{
  			Datum sliced_arr;
  			bool isnull = false;
  
! 			sliced_arr = makeMdArrayResult(astate, stmt->slice, 
! 									    ARR_DIMS(arr) + ARR_NDIM(arr) - stmt->slice, 
  									    ARR_LBOUND(arr) ? ARR_LBOUND(arr) + ARR_NDIM(arr) - stmt->slice : NULL,
  										    CurrentMemoryContext, true);
  			exec_assign_value(estate, ctrl_var, sliced_arr, valtype, &isnull);
--- 2190,2216 ----
  			}
  
  			if (stmt->slice > 0)
! 				astate = accumArrayResult(astate, value, isnull,
! 									array_typelem,
  									CurrentMemoryContext);
+ 
+ 			/* XXX: Why are we updating this here..? */
  			idx++;
  		}
  
! 		/*
! 		 * store an item in to the variable - we are expecting almost all
! 		 * iterations will be over scalar values, so we don't create
! 		 * a fake tuple over scalar and we don't use exec_move_row for
! 		 * scalars. This is about four times faster.
  		 */
  		if (astate != NULL)
  		{
  			Datum sliced_arr;
  			bool isnull = false;
  
! 			sliced_arr = makeMdArrayResult(astate, stmt->slice,
! 									    ARR_DIMS(arr) + ARR_NDIM(arr) - stmt->slice,
  									    ARR_LBOUND(arr) ? ARR_LBOUND(arr) + ARR_NDIM(arr) - stmt->slice : NULL,
  										    CurrentMemoryContext, true);
  			exec_assign_value(estate, ctrl_var, sliced_arr, valtype, &isnull);
*** a/src/pl/plpgsql/src/pl_funcs.c
--- b/src/pl/plpgsql/src/pl_funcs.c
***************
*** 605,611 **** static void
  dump_foreach_a(PLpgSQL_stmt_foreach_a *stmt)
  {
  	dump_ind();
! 	printf("FOREACHA %s", (stmt->rec != NULL) ? stmt->rec->refname : 
  						    (stmt->row != NULL) ? stmt->row->refname : stmt->var->refname);
  	if (stmt->slice != 0)
  		printf("SLICE %d ", stmt->slice);
--- 605,611 ----
  dump_foreach_a(PLpgSQL_stmt_foreach_a *stmt)
  {
  	dump_ind();
! 	printf("FOREACHA %s", (stmt->rec != NULL) ? stmt->rec->refname :
  						    (stmt->row != NULL) ? stmt->row->refname : stmt->var->refname);
  	if (stmt->slice != 0)
  		printf("SLICE %d ", stmt->slice);
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
***************
*** 436,442 **** typedef struct
  	PLpgSQL_rec *rec;
  	PLpgSQL_row *row;
  	PLpgSQL_expr	*expr;
! 	int		slice;
  	List	   *body;			/* List of statements */
  } PLpgSQL_stmt_foreach_a;
  
--- 436,442 ----
  	PLpgSQL_rec *rec;
  	PLpgSQL_row *row;
  	PLpgSQL_expr	*expr;
! 	int			slice;
  	List	   *body;			/* List of statements */
  } PLpgSQL_stmt_foreach_a;
  
*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
***************
*** 4283,4292 **** begin
  $$ language plpgsql;
  -- should fail
  select foreach_test(ARRAY[1,2,3,4]);
! ERROR:  target variable "x" for sliced array should be a array
  CONTEXT:  PL/pgSQL function "foreach_test" line 4 at FOREACH over array
  select foreach_test(ARRAY[[1,2],[3,4]]);
! ERROR:  target variable "x" for sliced array should be a array
  CONTEXT:  PL/pgSQL function "foreach_test" line 4 at FOREACH over array
  create or replace function foreach_test(anyarray)
  returns void as $$
--- 4283,4292 ----
  $$ language plpgsql;
  -- should fail
  select foreach_test(ARRAY[1,2,3,4]);
! ERROR:  target variable "x" for sliced array should be an array type
  CONTEXT:  PL/pgSQL function "foreach_test" line 4 at FOREACH over array
  select foreach_test(ARRAY[[1,2],[3,4]]);
! ERROR:  target variable "x" for sliced array should be an array type
  CONTEXT:  PL/pgSQL function "foreach_test" line 4 at FOREACH over array
  create or replace function foreach_test(anyarray)
  returns void as $$