Reject zero or negative BY step in plpgsql integer FOR-loops, and behave

sanely if the loop value overflows int32 on the way to the end value.
Avoid useless computation of "SELECT 1" when BY is omitted.  Avoid some
type-punning between Datum and int4 that dates from the original coding.

src/pl/plpgsql/src/gram.y

@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/gram.y,v 1.102 2007/04/29 01:21:09 neilc Exp $
+ *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/gram.y,v 1.103 2007/07/15 02:15:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -876,7 +876,7 @@ stmt_for		: opt_block_label K_FOR for_control loop_body
 						}
 
 						check_labels($1, $4.end_label);
-						/* close namespace started in opt_label */
+						/* close namespace started in opt_block_label */
 						plpgsql_ns_pop();
 					}
 				;
@@ -968,39 +968,25 @@ for_control		:
 								PLpgSQL_stmt_fori	*new;
 								char				*varname;
 
-								/* First expression is well-formed */
+								/* Check first expression is well-formed */
 								check_sql_expr(expr1->query);
 
-
-								expr2 = read_sql_construct(K_BY,
-														   K_LOOP,
+								/* Read and check the second one */
+								expr2 = read_sql_construct(K_LOOP,
+														   K_BY,
 														   "LOOP",
 														   "SELECT ",
 														   true,
-														   false,
+														   true,
 														   &tok);
 
+								/* Get the BY clause if any */
 								if (tok == K_BY)
 									expr_by = plpgsql_read_expression(K_LOOP, "LOOP");
 								else
-								{
-									/*
-									 * If there is no BY clause we will assume 1
-									 */
-									char buf[1024];
-									PLpgSQL_dstring		ds;
-
-									plpgsql_dstring_init(&ds);
-
-									expr_by = palloc0(sizeof(PLpgSQL_expr));
-									expr_by->dtype      		= PLPGSQL_DTYPE_EXPR;
-									strcpy(buf, "SELECT 1");
-									plpgsql_dstring_append(&ds, buf);
-									expr_by->query			    = pstrdup(plpgsql_dstring_get(&ds));
-									expr_by->plan				= NULL;
-								}
+									expr_by = NULL;
 
-								/* should have had a single variable name */
+								/* Should have had a single variable name */
 								plpgsql_error_lineno = $2.lineno;
 								if ($2.scalar && $2.row)
 									ereport(ERROR,
@@ -1023,7 +1009,7 @@ for_control		:
 								new->reverse  = reverse;
 								new->lower	  = expr1;
 								new->upper	  = expr2;
-								new->by		  = expr_by;
+								new->step	  = expr_by;
 
 								$$ = (PLpgSQL_stmt *) new;
 							}

src/pl/plpgsql/src/pl_exec.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.197 2007/06/05 21:31:08 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.198 2007/07/15 02:15:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1517,8 +1517,7 @@ exec_stmt_while(PLpgSQL_execstate *estate, PLpgSQL_stmt_while *stmt)
 /* ----------
  * exec_stmt_fori			Iterate an integer variable
  *					from a lower to an upper value
- *					incrementing or decrementing in BY value
- *					Loop can be left with exit.
+ *					incrementing or decrementing by the BY value
  * ----------
  */
 static int
@@ -1526,16 +1525,18 @@ exec_stmt_fori(PLpgSQL_execstate *estate, PLpgSQL_stmt_fori *stmt)
 {
 	PLpgSQL_var *var;
 	Datum		value;
-	Datum		by_value;
-	Oid			valtype;
 	bool		isnull;
+	Oid			valtype;
+	int32		loop_value;
+	int32		end_value;
+	int32		step_value;
 	bool		found = false;
 	int			rc = PLPGSQL_RC_OK;
 
 	var = (PLpgSQL_var *) (estate->datums[stmt->var->varno]);
 
 	/*
-	 * Get the value of the lower bound into the loop var
+	 * Get the value of the lower bound
 	 */
 	value = exec_eval_expr(estate, stmt->lower, &isnull, &valtype);
 	value = exec_cast_value(value, valtype, var->datatype->typoid,
@@ -1546,8 +1547,7 @@ exec_stmt_fori(PLpgSQL_execstate *estate, PLpgSQL_stmt_fori *stmt)
 		ereport(ERROR,
 				(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
 				 errmsg("lower bound of FOR loop cannot be NULL")));
-	var->value = value;
-	var->isnull = false;
+	loop_value = DatumGetInt32(value);
 	exec_eval_cleanup(estate);
 
 	/*
@@ -1562,22 +1562,32 @@ exec_stmt_fori(PLpgSQL_execstate *estate, PLpgSQL_stmt_fori *stmt)
 		ereport(ERROR,
 				(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
 				 errmsg("upper bound of FOR loop cannot be NULL")));
+	end_value = DatumGetInt32(value);
 	exec_eval_cleanup(estate);
 
 	/*
-	 * Get the by value
+	 * Get the step value
 	 */
-	by_value = exec_eval_expr(estate, stmt->by, &isnull, &valtype);
-	by_value = exec_cast_value(by_value, valtype, var->datatype->typoid,
-							   &(var->datatype->typinput),
-							   var->datatype->typioparam,
-							   var->datatype->atttypmod, isnull);
-
-	if (isnull)
-		ereport(ERROR,
-				(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
-				 errmsg("by value of FOR loop cannot be NULL")));
-	exec_eval_cleanup(estate);
+	if (stmt->step)
+	{
+		value = exec_eval_expr(estate, stmt->step, &isnull, &valtype);
+		value = exec_cast_value(value, valtype, var->datatype->typoid,
+								&(var->datatype->typinput),
+								var->datatype->typioparam,
+								var->datatype->atttypmod, isnull);
+		if (isnull)
+			ereport(ERROR,
+					(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
+					 errmsg("BY value of FOR loop cannot be NULL")));
+		step_value = DatumGetInt32(value);
+		exec_eval_cleanup(estate);
+		if (step_value <= 0)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("BY value of FOR loop must be greater than zero")));
+	}
+	else
+		step_value = 1;
 
 	/*
 	 * Now do the loop
@@ -1585,21 +1595,27 @@ exec_stmt_fori(PLpgSQL_execstate *estate, PLpgSQL_stmt_fori *stmt)
 	for (;;)
 	{
 		/*
-		 * Check bounds
+		 * Check against upper bound
 		 */
 		if (stmt->reverse)
 		{
-			if ((int4) (var->value) < (int4) value)
+			if (loop_value < end_value)
 				break;
 		}
 		else
 		{
-			if ((int4) (var->value) > (int4) value)
+			if (loop_value > end_value)
 				break;
 		}
 
 		found = true;			/* looped at least once */
 
+		/*
+		 * Assign current value to loop var
+		 */
+		var->value = Int32GetDatum(loop_value);
+		var->isnull = false;
+
 		/*
 		 * Execute the statements
 		 */
@@ -1625,13 +1641,12 @@ exec_stmt_fori(PLpgSQL_execstate *estate, PLpgSQL_stmt_fori *stmt)
 			 * current statement's label, if any: return RC_EXIT so that the
 			 * EXIT continues to propagate up the stack.
 			 */
-
 			break;
 		}
 		else if (rc == PLPGSQL_RC_CONTINUE)
 		{
 			if (estate->exitlabel == NULL)
-				/* anonymous continue, so re-run the current loop */
+				/* unlabelled continue, so re-run the current loop */
 				rc = PLPGSQL_RC_OK;
 			else if (stmt->label != NULL &&
 					 strcmp(stmt->label, estate->exitlabel) == 0)
@@ -1652,12 +1667,21 @@ exec_stmt_fori(PLpgSQL_execstate *estate, PLpgSQL_stmt_fori *stmt)
 		}
 
 		/*
-		 * Increase/decrease loop var
+		 * Increase/decrease loop value, unless it would overflow, in which
+		 * case exit the loop.
 		 */
 		if (stmt->reverse)
-			var->value -= by_value;
+		{
+			if ((int32) (loop_value - step_value) > loop_value)
+				break;
+			loop_value -= step_value;
+		}
 		else
-			var->value += by_value;
+		{
+			if ((int32) (loop_value + step_value) < loop_value)
+				break;
+			loop_value += step_value;
+		}
 	}
 
 	/*

src/pl/plpgsql/src/pl_funcs.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_funcs.c,v 1.59 2007/04/29 01:21:09 neilc Exp $
+ *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_funcs.c,v 1.60 2007/07/15 02:15:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -701,8 +701,8 @@ dump_fori(PLpgSQL_stmt_fori *stmt)
 	dump_expr(stmt->upper);
 	printf("\n");
 	dump_ind();
-	printf("    by = ");
-	dump_expr(stmt->by);
+	printf("    step = ");
+	dump_expr(stmt->step);
 	printf("\n");
 	dump_indent -= 2;
 

src/pl/plpgsql/src/plpgsql.h

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.88 2007/04/29 01:21:09 neilc Exp $
+ *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.89 2007/07/15 02:15:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -402,7 +402,7 @@ typedef struct
 	PLpgSQL_var *var;
 	PLpgSQL_expr *lower;
 	PLpgSQL_expr *upper;
-	PLpgSQL_expr *by;
+	PLpgSQL_expr *step;			/* NULL means default (ie, BY 1) */
 	int			reverse;
 	List	   *body;			/* List of statements */
 } PLpgSQL_stmt_fori;