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;