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.

diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
index 164a2179a31e78365958016a7f3e9a21bbc0e1f7..0f41c999724b5726fad8710064ecd7df0d74cf56 100644 (file)
--- a/src/pl/plpgsql/src/gram.y
+++ b/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;
                                                        }

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 2582935452d0150c76ff429476c414f8fa00b4d8..9527fdc61d5aa9c3324acf1ebdf004717c596209 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/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;
+               }
        }
 
        /*

diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index c344c9e4ea7ee32f7bb9acb4af842f854ba760cf..4d5eba73e3b43e7c3c96f74e6844cff37d7b1f3c 100644 (file)
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/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;
 

diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 20ee074564a101d23eba4fcbfc269bb8eb1e168e..dce7be2b1bd29fe3f09c71d45812ed6b08e13dca 100644 (file)
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/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;