Okay, I've had it with answering newbie questions about why plpgsql
authorTom Lane <tgl@sss.pgh.pa.us>
Sun, 4 Jul 2004 02:49:04 +0000 (02:49 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Sun, 4 Jul 2004 02:49:04 +0000 (02:49 +0000)
FOR loops are giving weird syntax errors.  Restructure parsing of FOR
loops so that the integer-loop-vs-query-loop decision is driven off
the presence of '..' between IN and LOOP, rather than the presence
of a matching record/row variable name.  Hopefully this will make the
behavior a bit more transparent.

doc/src/sgml/plpgsql.sgml
src/pl/plpgsql/src/gram.y

index b8eadd04552e328d93c879c08fec0c2036bd52b9..952351170cc2c3e161aedc866ccb507b84b0e37e 100644 (file)
@@ -1,5 +1,5 @@
 <!--
-$PostgreSQL: pgsql/doc/src/sgml/plpgsql.sgml,v 1.38 2004/05/16 23:22:06 neilc Exp $
+$PostgreSQL: pgsql/doc/src/sgml/plpgsql.sgml,v 1.39 2004/07/04 02:48:52 tgl Exp $
 -->
 
 <chapter id="plpgsql"> 
@@ -1769,7 +1769,7 @@ END;
 
     <para>
      The <literal>FOR-IN-EXECUTE</> statement is another way to iterate over
-     records:
+     rows:
 <synopsis>
 <optional>&lt;&lt;<replaceable>label</replaceable>&gt;&gt;</optional>
 FOR <replaceable>record_or_row</replaceable> IN EXECUTE <replaceable>text_expression</replaceable> LOOP 
@@ -1788,13 +1788,12 @@ END LOOP;
     <para>
      The <application>PL/pgSQL</> parser presently distinguishes the
      two kinds of <literal>FOR</> loops (integer or query result) by checking
-     whether the target variable mentioned just after <literal>FOR</> has been
-     declared as a record or row variable.  If not, it's presumed to be
-     an integer <literal>FOR</> loop.  This can cause rather nonintuitive error
-     messages when the true problem is, say, that one has
-     misspelled the variable name after the <literal>FOR</>.  Typically
-     the complaint will be something like <literal>missing ".." at end of SQL
-     expression</>.
+     whether <literal>..</> appears outside any parentheses between
+     <literal>IN</> and <literal>LOOP</>.  If <literal>..</> is not seen then
+     the loop is presumed to be a loop over rows.  Mistyping the <literal>..</>
+     is thus likely to lead to a complaint along the lines of
+     <quote>loop variable of loop over rows must be a record or row</>,
+     rather than the simple syntax error one might expect to get.
     </para>
     </note>
   </sect2>
index 826e1539d1e25cb68f4ae24b2f9586992c7f73fd..9b70d944ab98aea9bba481ca119c7b428ad5955f 100644 (file)
@@ -4,7 +4,7 @@
  *                       procedural language
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/pl/plpgsql/src/gram.y,v 1.56 2004/06/04 02:37:06 tgl Exp $
+ *   $PostgreSQL: pgsql/src/pl/plpgsql/src/gram.y,v 1.57 2004/07/04 02:49:04 tgl Exp $
  *
  *   This software is copyrighted by Jan Wieck - Hamburg.
  *
 
 
 static PLpgSQL_expr    *read_sql_construct(int until,
+                                           int until2,
                                            const char *expected,
                                            bool isexpression,
-                                           const char *sqlstart);
+                                           const char *sqlstart,
+                                           int *endtoken);
 static PLpgSQL_expr    *read_sql_stmt(const char *sqlstart);
 static PLpgSQL_type    *read_datatype(int tok);
 static PLpgSQL_stmt    *make_select_stmt(void);
@@ -73,9 +75,11 @@ static   void check_assignable(PLpgSQL_datum *datum);
        }                       dtlist;
        struct
        {
-           int  reverse;
-           PLpgSQL_expr *expr;
-       }                       forilow;
+           char *name;
+           int  lineno;
+           PLpgSQL_rec *rec;
+           PLpgSQL_row *row;
+       }                       forvariable;
        struct
        {
            char *label;
@@ -110,11 +114,10 @@ static    void check_assignable(PLpgSQL_datum *datum);
 %type <expr>   opt_exitcond
 
 %type <ival>   assign_var cursor_variable
-%type <var>        fori_var cursor_varptr
+%type <var>        cursor_varptr
 %type <variable>   decl_cursor_arg
-%type <varname> fori_varname
-%type <forilow> fori_lower
-%type <rec>        fors_target
+%type <forvariable>    for_variable
+%type <stmt>   for_control
 
 %type <str>        opt_lblname opt_label
 %type <str>        opt_exitlabel
@@ -124,8 +127,8 @@ static  void check_assignable(PLpgSQL_datum *datum);
 %type <stmt>   proc_stmt pl_block
 %type <stmt>   stmt_assign stmt_if stmt_loop stmt_while stmt_exit
 %type <stmt>   stmt_return stmt_return_next stmt_raise stmt_execsql
-%type <stmt>   stmt_fori stmt_fors stmt_select stmt_perform
-%type <stmt>   stmt_dynexecute stmt_dynfors stmt_getdiag
+%type <stmt>   stmt_for stmt_select stmt_perform
+%type <stmt>   stmt_dynexecute stmt_getdiag
 %type <stmt>   stmt_open stmt_fetch stmt_close
 
 %type <intlist>    raise_params
@@ -616,9 +619,7 @@ proc_stmt       : pl_block ';'
                        { $$ = $1; }
                | stmt_while
                        { $$ = $1; }
-               | stmt_fori
-                       { $$ = $1; }
-               | stmt_fors
+               | stmt_for
                        { $$ = $1; }
                | stmt_select
                        { $$ = $1; }
@@ -634,8 +635,6 @@ proc_stmt       : pl_block ';'
                        { $$ = $1; }
                | stmt_dynexecute
                        { $$ = $1; }
-               | stmt_dynfors
-                       { $$ = $1; }
                | stmt_perform
                        { $$ = $1; }
                | stmt_getdiag
@@ -882,152 +881,218 @@ stmt_while      : opt_label K_WHILE lno expr_until_loop loop_body
                    }
                ;
 
-stmt_fori      : opt_label K_FOR lno fori_var K_IN fori_lower expr_until_loop loop_body
+stmt_for       : opt_label K_FOR for_control loop_body
                    {
-                       PLpgSQL_stmt_fori       *new;
+                       /* This runs after we've scanned the loop body */
+                       if ($3->cmd_type == PLPGSQL_STMT_FORI)
+                       {
+                           PLpgSQL_stmt_fori       *new;
 
-                       new = malloc(sizeof(PLpgSQL_stmt_fori));
-                       memset(new, 0, sizeof(PLpgSQL_stmt_fori));
+                           new = (PLpgSQL_stmt_fori *) $3;
+                           new->label    = $1;
+                           new->body     = $4;
+                           $$ = (PLpgSQL_stmt *) new;
+                       }
+                       else if ($3->cmd_type == PLPGSQL_STMT_FORS)
+                       {
+                           PLpgSQL_stmt_fors       *new;
 
-                       new->cmd_type = PLPGSQL_STMT_FORI;
-                       new->lineno   = $3;
-                       new->label    = $1;
-                       new->var      = $4;
-                       new->reverse  = $6.reverse;
-                       new->lower    = $6.expr;
-                       new->upper    = $7;
-                       new->body     = $8;
+                           new = (PLpgSQL_stmt_fors *) $3;
+                           new->label    = $1;
+                           new->body     = $4;
+                           $$ = (PLpgSQL_stmt *) new;
+                       }
+                       else
+                       {
+                           PLpgSQL_stmt_dynfors    *new;
 
-                       plpgsql_ns_pop();
+                           Assert($3->cmd_type == PLPGSQL_STMT_DYNFORS);
+                           new = (PLpgSQL_stmt_dynfors *) $3;
+                           new->label    = $1;
+                           new->body     = $4;
+                           $$ = (PLpgSQL_stmt *) new;
+                       }
 
-                       $$ = (PLpgSQL_stmt *)new;
+                       /* close namespace started in opt_label */
+                       plpgsql_ns_pop();
                    }
                ;
 
-fori_var       : fori_varname
+for_control        : lno for_variable K_IN
                    {
-                       PLpgSQL_var     *new;
+                       int         tok;
+                       bool        reverse = false;
+                       bool        execute = false;
+                       PLpgSQL_expr *expr1;
 
-                       new = (PLpgSQL_var *)
-                           plpgsql_build_variable($1.name, $1.lineno,
-                                                  plpgsql_build_datatype(INT4OID,
-                                                                         -1),
-                                                  true);
+                       /* check for REVERSE and EXECUTE */
+                       tok = yylex();
+                       if (tok == K_REVERSE)
+                       {
+                           reverse = true;
+                           tok = yylex();
+                       }
 
-                       plpgsql_add_initdatums(NULL);
+                       if (tok == K_EXECUTE)
+                           execute = true;
+                       else
+                           plpgsql_push_back_token(tok);
 
-                       $$ = new;
-                   }
-               ;
+                       /* Collect one or two expressions */
+                       expr1 = read_sql_construct(K_DOTDOT,
+                                                  K_LOOP,
+                                                  "LOOP",
+                                                  true,
+                                                  "SELECT ",
+                                                  &tok);
 
-fori_varname   : T_SCALAR
-                   {
-                       char    *name;
+                       if (tok == K_DOTDOT)
+                       {
+                           /* Found .., so it must be an integer loop */
+                           PLpgSQL_stmt_fori   *new;
+                           PLpgSQL_expr        *expr2;
+                           PLpgSQL_var         *fvar;
 
-                       plpgsql_convert_ident(yytext, &name, 1);
-                       /* name should be malloc'd for use as varname */
-                       $$.name = strdup(name);
-                       $$.lineno  = plpgsql_scanner_lineno();
-                       pfree(name);
-                   }
-               | T_WORD
-                   {
-                       char    *name;
+                           expr2 = plpgsql_read_expression(K_LOOP, "LOOP");
 
-                       plpgsql_convert_ident(yytext, &name, 1);
-                       /* name should be malloc'd for use as varname */
-                       $$.name = strdup(name);
-                       $$.lineno  = plpgsql_scanner_lineno();
-                       pfree(name);
-                   }
-               ;
+                           if (execute)
+                           {
+                               plpgsql_error_lineno = $1;
+                               yyerror("cannot specify EXECUTE in integer for-loop");
+                           }
 
-fori_lower     :
-                   {
-                       int         tok;
+                           /* name should be malloc'd for use as varname */
+                           fvar = (PLpgSQL_var *)
+                               plpgsql_build_variable(strdup($2.name),
+                                                      $2.lineno,
+                                                      plpgsql_build_datatype(INT4OID,
+                                                                             -1),
+                                                      true);
 
-                       tok = yylex();
-                       if (tok == K_REVERSE)
-                       {
-                           $$.reverse = 1;
+                           /* put the for-variable into the local block */
+                           plpgsql_add_initdatums(NULL);
+
+                           new = malloc(sizeof(PLpgSQL_stmt_fori));
+                           memset(new, 0, sizeof(PLpgSQL_stmt_fori));
+
+                           new->cmd_type = PLPGSQL_STMT_FORI;
+                           new->lineno   = $1;
+                           new->var      = fvar;
+                           new->reverse  = reverse;
+                           new->lower    = expr1;
+                           new->upper    = expr2;
+
+                           $$ = (PLpgSQL_stmt *) new;
                        }
-                       else
+                       else if (execute)
                        {
-                           $$.reverse = 0;
-                           plpgsql_push_back_token(tok);
-                       }
+                           /* No .., so it must be a loop over rows */
+                           PLpgSQL_stmt_dynfors    *new;
 
-                       $$.expr = plpgsql_read_expression(K_DOTDOT, "..");
-                   }
-               ;
+                           if (reverse)
+                           {
+                               plpgsql_error_lineno = $1;
+                               yyerror("cannot specify REVERSE in loop over rows");
+                           }
 
-stmt_fors      : opt_label K_FOR lno fors_target K_IN K_SELECT expr_until_loop loop_body
-                   {
-                       PLpgSQL_stmt_fors       *new;
+                           new = malloc(sizeof(PLpgSQL_stmt_dynfors));
+                           memset(new, 0, sizeof(PLpgSQL_stmt_dynfors));
 
-                       new = malloc(sizeof(PLpgSQL_stmt_fors));
-                       memset(new, 0, sizeof(PLpgSQL_stmt_fors));
+                           new->cmd_type = PLPGSQL_STMT_DYNFORS;
+                           new->lineno   = $1;
+                           if ($2.rec)
+                               new->rec = $2.rec;
+                           else if ($2.row)
+                               new->row = $2.row;
+                           else
+                           {
+                               plpgsql_error_lineno = $1;
+                               yyerror("loop variable of loop over rows must be a record or row variable");
+                           }
+                           new->query = expr1;
 
-                       new->cmd_type = PLPGSQL_STMT_FORS;
-                       new->lineno   = $3;
-                       new->label    = $1;
-                       switch ($4->dtype)
-                       {
-                           case PLPGSQL_DTYPE_REC:
-                               new->rec = $4;
-                               break;
-                           case PLPGSQL_DTYPE_ROW:
-                               new->row = (PLpgSQL_row *)$4;
-                               break;
-                           default:
-                               elog(ERROR, "unrecognized dtype: %d",
-                                    $4->dtype);
+                           $$ = (PLpgSQL_stmt *) new;
                        }
-                       new->query = $7;
-                       new->body  = $8;
+                       else
+                       {
+                           /* No .., so it must be a loop over rows */
+                           PLpgSQL_stmt_fors       *new;
+                           char                    *newquery;
 
-                       plpgsql_ns_pop();
+                           if (reverse)
+                           {
+                               plpgsql_error_lineno = $1;
+                               yyerror("cannot specify REVERSE in loop over rows");
+                           }
 
-                       $$ = (PLpgSQL_stmt *)new;
-                   }
-               ;
+                           new = malloc(sizeof(PLpgSQL_stmt_fors));
+                           memset(new, 0, sizeof(PLpgSQL_stmt_fors));
 
-stmt_dynfors : opt_label K_FOR lno fors_target K_IN K_EXECUTE expr_until_loop loop_body
-                   {
-                       PLpgSQL_stmt_dynfors    *new;
+                           new->cmd_type = PLPGSQL_STMT_FORS;
+                           new->lineno   = $1;
+                           if ($2.rec)
+                               new->rec = $2.rec;
+                           else if ($2.row)
+                               new->row = $2.row;
+                           else
+                           {
+                               plpgsql_error_lineno = $1;
+                               yyerror("loop variable of loop over rows must be a record or row variable");
+                           }
+                           /*
+                            * Must get rid of the "SELECT " we prepended
+                            * to expr1's text
+                            */
+                           newquery = strdup(expr1->query + 7);
+                           free(expr1->query);
+                           expr1->query = newquery;
 
-                       new = malloc(sizeof(PLpgSQL_stmt_dynfors));
-                       memset(new, 0, sizeof(PLpgSQL_stmt_dynfors));
+                           new->query = expr1;
 
-                       new->cmd_type = PLPGSQL_STMT_DYNFORS;
-                       new->lineno   = $3;
-                       new->label    = $1;
-                       switch ($4->dtype)
-                       {
-                           case PLPGSQL_DTYPE_REC:
-                               new->rec = $4;
-                               break;
-                           case PLPGSQL_DTYPE_ROW:
-                               new->row = (PLpgSQL_row *)$4;
-                               break;
-                           default:
-                               elog(ERROR, "unrecognized dtype: %d",
-                                    $4->dtype);
+                           $$ = (PLpgSQL_stmt *) new;
                        }
-                       new->query = $7;
-                       new->body  = $8;
+                   }
+               ;
 
-                       plpgsql_ns_pop();
+for_variable   : T_SCALAR
+                   {
+                       char        *name;
 
-                       $$ = (PLpgSQL_stmt *)new;
+                       plpgsql_convert_ident(yytext, &name, 1);
+                       $$.name = name;
+                       $$.lineno  = plpgsql_scanner_lineno();
+                       $$.rec = NULL;
+                       $$.row = NULL;
                    }
-               ;
+               | T_WORD
+                   {
+                       char        *name;
+
+                       plpgsql_convert_ident(yytext, &name, 1);
+                       $$.name = name;
+                       $$.lineno  = plpgsql_scanner_lineno();
+                       $$.rec = NULL;
+                       $$.row = NULL;
+                   }
+               | T_RECORD
+                   {
+                       char        *name;
 
-fors_target        : T_RECORD
-                   { $$ = yylval.rec; }
+                       plpgsql_convert_ident(yytext, &name, 1);
+                       $$.name = name;
+                       $$.lineno  = plpgsql_scanner_lineno();
+                       $$.rec = yylval.rec;
+                       $$.row = NULL;
+                   }
                | T_ROW
                    {
-                       $$ = (PLpgSQL_rec *)(yylval.row);
+                       char        *name;
+
+                       plpgsql_convert_ident(yytext, &name, 1);
+                       $$.name = name;
+                       $$.lineno  = plpgsql_scanner_lineno();
+                       $$.row = yylval.row;
+                       $$.rec = NULL;
                    }
                ;
 
@@ -1521,20 +1586,33 @@ lno             :
 PLpgSQL_expr *
 plpgsql_read_expression(int until, const char *expected)
 {
-   return read_sql_construct(until, expected, true, "SELECT ");
+   return read_sql_construct(until, 0, expected, true, "SELECT ", NULL);
 }
 
 static PLpgSQL_expr *
 read_sql_stmt(const char *sqlstart)
 {
-   return read_sql_construct(';', ";", false, sqlstart);
+   return read_sql_construct(';', 0, ";", false, sqlstart, NULL);
 }
 
+/*
+ * Read a SQL construct and build a PLpgSQL_expr for it.
+ *
+ * until:      token code for expected terminator
+ * until2:     token code for alternate terminator (pass 0 if none)
+ * expected:   text to use in complaining that terminator was not found
+ * isexpression: whether to say we're reading an "expression" or a "statement"
+ * sqlstart:   text to prefix to the accumulated SQL text
+ * endtoken:   if not NULL, ending token is stored at *endtoken
+ *             (this is only interesting if until2 isn't zero)
+ */
 static PLpgSQL_expr *
 read_sql_construct(int until,
+                  int until2,
                   const char *expected,
                   bool isexpression,
-                  const char *sqlstart)
+                  const char *sqlstart,
+                  int *endtoken)
 {
    int                 tok;
    int                 lno;
@@ -1554,6 +1632,8 @@ read_sql_construct(int until,
        tok = yylex();
        if (tok == until && parenlevel == 0)
            break;
+       if (tok == until2 && parenlevel == 0)
+           break;
        if (tok == '(' || tok == '[')
            parenlevel++;
        else if (tok == ')' || tok == ']')
@@ -1586,7 +1666,6 @@ read_sql_construct(int until,
                        (errcode(ERRCODE_SYNTAX_ERROR),
                         errmsg("missing \"%s\" at end of SQL statement",
                                expected)));
-           break;
        }
        if (plpgsql_SpaceScanned)
            plpgsql_dstring_append(&ds, " ");
@@ -1616,6 +1695,9 @@ read_sql_construct(int until,
        }
    }
 
+   if (endtoken)
+       *endtoken = tok;
+
    expr = malloc(sizeof(PLpgSQL_expr) + sizeof(int) * nparams - sizeof(int));
    expr->dtype         = PLPGSQL_DTYPE_EXPR;
    expr->query         = strdup(plpgsql_dstring_get(&ds));