Re: pl/pgsql: END verbosity [patch] - Mailing list pgsql-hackers
| From | Neil Conway |
|---|---|
| Subject | Re: pl/pgsql: END verbosity [patch] |
| Date | |
| Msg-id | 42C539BF.8040505@samurai.com Whole thread Raw |
| In response to | pl/pgsql: END verbosity [patch] (Pavel Stehule <stehule@kix.fsv.cvut.cz>) |
| Responses |
Re: [PATCHES] pl/pgsql: END verbosity [patch]
|
| List | pgsql-hackers |
Pavel Stehule wrote:
> this patch allows optional using label with END and END LOOP. Ending label
> has only informational value, but can enhance readability large block and
> enhance likeness with Oracle.
>
> <<main>>LOOP
> ...
> ...
> END LOOP<<main>>;
Attached is a revised version of this patch. Changes / comments:
- AFAICS Oracle's syntax is actually
<<label>> LOOP
...
END LOOP label;
i.e. the ending block label isn't enclosed in <<>>. I've adjusted the
patch accordingly.
- your patch broke EXIT and CONTINUE, as running the regression tests
would have made clear.
- yyerror() will set plpgsql_error_lineno, so you needn't do it
yourself. I changed it to use ereport(ERROR) anyway, as it seems a bit
more appropriate. I'm not quite happy with the error message text:
ERROR: end label "outer_label" differs from block's label "inner_label"
CONTEXT: compile of PL/pgSQL function "end_label3" near line 6
ERROR: end label "outer_label" specified for unlabelled block
CONTEXT: compile of PL/pgSQL function "end_label4" near line 5
suggestions for improvement are welcome.
BTW, I notice that some but not all the call sites of ereport(ERROR) in
PL/PgSQL's gram.y set plpgsql_error_lineno. Is there a reason for this?
Barring any objections, I'll apply the attached patch to CVS tomorrow.
-Neil
Index: doc/src/sgml/plpgsql.sgml
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/doc/src/sgml/plpgsql.sgml,v
retrieving revision 1.74
diff -c -r1.74 plpgsql.sgml
*** doc/src/sgml/plpgsql.sgml 22 Jun 2005 01:35:02 -0000 1.74
--- doc/src/sgml/plpgsql.sgml 1 Jul 2005 11:43:36 -0000
***************
*** 456,462 ****
<replaceable>declarations</replaceable> </optional>
BEGIN
<replaceable>statements</replaceable>
! END;
</synopsis>
</para>
--- 456,462 ----
<replaceable>declarations</replaceable> </optional>
BEGIN
<replaceable>statements</replaceable>
! END <optional> <replaceable>label</replaceable> </optional>;
</synopsis>
</para>
***************
*** 1789,1806 ****
<title><literal>LOOP</></title>
<synopsis>
! <optional><<<replaceable>label</replaceable>>></optional>
LOOP
<replaceable>statements</replaceable>
! END LOOP;
</synopsis>
<para>
! <literal>LOOP</> defines an unconditional loop that is repeated indefinitely
! until terminated by an <literal>EXIT</> or <command>RETURN</command>
! statement. The optional label can be used by <literal>EXIT</> statements in
! nested loops to specify which level of nesting should be
! terminated.
</para>
</sect3>
--- 1789,1807 ----
<title><literal>LOOP</></title>
<synopsis>
! <optional> <<<replaceable>label</replaceable>>> </optional>
LOOP
<replaceable>statements</replaceable>
! END LOOP <optional> <replaceable>label</replaceable> </optional>;
</synopsis>
<para>
! <literal>LOOP</> defines an unconditional loop that is repeated
! indefinitely until terminated by an <literal>EXIT</> or
! <command>RETURN</command> statement. The optional
! <replaceable>label</replaceable> can be used by <literal>EXIT</>
! and <literal>CONTINUE</literal> statements in nested loops to
! specify which loop the statement should be applied to.
</para>
</sect3>
***************
*** 1920,1929 ****
</indexterm>
<synopsis>
! <optional><<<replaceable>label</replaceable>>></optional>
WHILE <replaceable>expression</replaceable> LOOP
<replaceable>statements</replaceable>
! END LOOP;
</synopsis>
<para>
--- 1921,1930 ----
</indexterm>
<synopsis>
! <optional> <<<replaceable>label</replaceable>>> </optional>
WHILE <replaceable>expression</replaceable> LOOP
<replaceable>statements</replaceable>
! END LOOP <optional> <replaceable>label</replaceable> </optional>;
</synopsis>
<para>
***************
*** 1951,1960 ****
<title><literal>FOR</> (integer variant)</title>
<synopsis>
! <optional><<<replaceable>label</replaceable>>></optional>
FOR <replaceable>name</replaceable> IN <optional> REVERSE </optional> <replaceable>expression</replaceable> ..
<replaceable>expression</replaceable>LOOP
<replaceable>statements</replaceable>
! END LOOP;
</synopsis>
<para>
--- 1952,1961 ----
<title><literal>FOR</> (integer variant)</title>
<synopsis>
! <optional> <<<replaceable>label</replaceable>>> </optional>
FOR <replaceable>name</replaceable> IN <optional> REVERSE </optional> <replaceable>expression</replaceable> ..
<replaceable>expression</replaceable>LOOP
<replaceable>statements</replaceable>
! END LOOP <optional> <replaceable>labal</replaceable> </optional>;
</synopsis>
<para>
***************
*** 1997,2006 ****
the results of a query and manipulate that data
accordingly. The syntax is:
<synopsis>
! <optional><<<replaceable>label</replaceable>>></optional>
FOR <replaceable>record_or_row</replaceable> IN <replaceable>query</replaceable> LOOP
<replaceable>statements</replaceable>
! END LOOP;
</synopsis>
The record or row variable is successively assigned each row
resulting from the <replaceable>query</replaceable> (which must be a
--- 1998,2007 ----
the results of a query and manipulate that data
accordingly. The syntax is:
<synopsis>
! <optional> <<<replaceable>label</replaceable>>> </optional>
FOR <replaceable>record_or_row</replaceable> IN <replaceable>query</replaceable> LOOP
<replaceable>statements</replaceable>
! END LOOP <optional> <replaceable>label</replaceable> </optional>;
</synopsis>
The record or row variable is successively assigned each row
resulting from the <replaceable>query</replaceable> (which must be a
***************
*** 2036,2045 ****
The <literal>FOR-IN-EXECUTE</> statement is another way to iterate over
rows:
<synopsis>
! <optional><<<replaceable>label</replaceable>>></optional>
FOR <replaceable>record_or_row</replaceable> IN EXECUTE <replaceable>text_expression</replaceable> LOOP
<replaceable>statements</replaceable>
! END LOOP;
</synopsis>
This is like the previous form, except that the source
<command>SELECT</command> statement is specified as a string
--- 2037,2046 ----
The <literal>FOR-IN-EXECUTE</> statement is another way to iterate over
rows:
<synopsis>
! <optional> <<<replaceable>label</replaceable>>> </optional>
FOR <replaceable>record_or_row</replaceable> IN EXECUTE <replaceable>text_expression</replaceable> LOOP
<replaceable>statements</replaceable>
! END LOOP <optional> <replaceable>label</replaceable> </optional>;
</synopsis>
This is like the previous form, except that the source
<command>SELECT</command> statement is specified as a string
Index: src/pl/plpgsql/src/gram.y
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/src/pl/plpgsql/src/gram.y,v
retrieving revision 1.77
diff -c -r1.77 gram.y
*** src/pl/plpgsql/src/gram.y 22 Jun 2005 01:35:02 -0000 1.77
--- src/pl/plpgsql/src/gram.y 1 Jul 2005 12:08:45 -0000
***************
*** 56,61 ****
--- 56,63 ----
PLpgSQL_datum *initial_datum);
static void check_sql_expr(const char *stmt);
static void plpgsql_sql_error_callback(void *arg);
+ static void check_labels(const char *start_label,
+ const char *end_label);
%}
***************
*** 69,75 ****
int lineno;
} varname;
struct
! {
char *name;
int lineno;
PLpgSQL_rec *rec;
--- 71,77 ----
int lineno;
} varname;
struct
! {
char *name;
int lineno;
PLpgSQL_rec *rec;
***************
*** 81,86 ****
--- 83,93 ----
int n_initvars;
int *initvarnos;
} declhdr;
+ struct
+ {
+ char *end_label;
+ List *stmts;
+ } loop_body;
List *list;
PLpgSQL_type *dtype;
PLpgSQL_datum *scalar; /* a VAR, RECFIELD, or TRIGARG */
***************
*** 119,129 ****
%type <forvariable> for_variable
%type <stmt> for_control
! %type <str> opt_lblname opt_label
! %type <str> opt_exitlabel
%type <str> execsql_start
! %type <list> proc_sect proc_stmts stmt_else loop_body
%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
--- 126,136 ----
%type <forvariable> for_variable
%type <stmt> for_control
! %type <str> opt_lblname opt_block_label opt_label
%type <str> execsql_start
! %type <list> proc_sect proc_stmts stmt_else
! %type <loop_body> loop_body
%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
***************
*** 248,257 ****
| ';'
;
! pl_block : decl_sect K_BEGIN lno proc_sect exception_sect K_END
{
PLpgSQL_stmt_block *new;
new = palloc0(sizeof(PLpgSQL_stmt_block));
new->cmd_type = PLPGSQL_STMT_BLOCK;
--- 255,266 ----
| ';'
;
! pl_block : decl_sect K_BEGIN lno proc_sect exception_sect K_END opt_label
{
PLpgSQL_stmt_block *new;
+ check_labels($1.label, $7);
+
new = palloc0(sizeof(PLpgSQL_stmt_block));
new->cmd_type = PLPGSQL_STMT_BLOCK;
***************
*** 269,275 ****
;
! decl_sect : opt_label
{
plpgsql_ns_setlocal(false);
$$.label = $1;
--- 278,284 ----
;
! decl_sect : opt_block_label
{
plpgsql_ns_setlocal(false);
$$.label = $1;
***************
*** 277,283 ****
$$.initvarnos = NULL;
plpgsql_add_initdatums(NULL);
}
! | opt_label decl_start
{
plpgsql_ns_setlocal(false);
$$.label = $1;
--- 286,292 ----
$$.initvarnos = NULL;
plpgsql_add_initdatums(NULL);
}
! | opt_block_label decl_start
{
plpgsql_ns_setlocal(false);
$$.label = $1;
***************
*** 285,291 ****
$$.initvarnos = NULL;
plpgsql_add_initdatums(NULL);
}
! | opt_label decl_start decl_stmts
{
plpgsql_ns_setlocal(false);
if ($3 != NULL)
--- 294,300 ----
$$.initvarnos = NULL;
plpgsql_add_initdatums(NULL);
}
! | opt_block_label decl_start decl_stmts
{
plpgsql_ns_setlocal(false);
if ($3 != NULL)
***************
*** 409,415 ****
plpgsql_ns_setlocal(false);
query = read_sql_stmt("");
plpgsql_ns_setlocal(true);
!
$$ = query;
}
;
--- 418,424 ----
plpgsql_ns_setlocal(false);
query = read_sql_stmt("");
plpgsql_ns_setlocal(true);
!
$$ = query;
}
;
***************
*** 757,763 ****
* ... ...
* ELSE ELSE
* ... ...
! * END IF END IF
* END IF
*/
PLpgSQL_stmt_if *new_if;
--- 766,772 ----
* ... ...
* ELSE ELSE
* ... ...
! * END IF END IF
* END IF
*/
PLpgSQL_stmt_if *new_if;
***************
*** 776,786 ****
| K_ELSE proc_sect
{
! $$ = $2;
}
;
! stmt_loop : opt_label K_LOOP lno loop_body
{
PLpgSQL_stmt_loop *new;
--- 785,795 ----
| K_ELSE proc_sect
{
! $$ = $2;
}
;
! stmt_loop : opt_block_label K_LOOP lno loop_body
{
PLpgSQL_stmt_loop *new;
***************
*** 788,802 ****
new->cmd_type = PLPGSQL_STMT_LOOP;
new->lineno = $3;
new->label = $1;
! new->body = $4;
plpgsql_ns_pop();
$$ = (PLpgSQL_stmt *)new;
}
;
! stmt_while : opt_label K_WHILE lno expr_until_loop loop_body
{
PLpgSQL_stmt_while *new;
--- 797,812 ----
new->cmd_type = PLPGSQL_STMT_LOOP;
new->lineno = $3;
new->label = $1;
! new->body = $4.stmts;
+ check_labels($1, $4.end_label);
plpgsql_ns_pop();
$$ = (PLpgSQL_stmt *)new;
}
;
! stmt_while : opt_block_label K_WHILE lno expr_until_loop loop_body
{
PLpgSQL_stmt_while *new;
***************
*** 805,819 ****
new->lineno = $3;
new->label = $1;
new->cond = $4;
! new->body = $5;
plpgsql_ns_pop();
$$ = (PLpgSQL_stmt *)new;
}
;
! stmt_for : opt_label K_FOR for_control loop_body
{
/* This runs after we've scanned the loop body */
if ($3->cmd_type == PLPGSQL_STMT_FORI)
--- 815,830 ----
new->lineno = $3;
new->label = $1;
new->cond = $4;
! new->body = $5.stmts;
+ check_labels($1, $5.end_label);
plpgsql_ns_pop();
$$ = (PLpgSQL_stmt *)new;
}
;
! stmt_for : opt_block_label K_FOR for_control loop_body
{
/* This runs after we've scanned the loop body */
if ($3->cmd_type == PLPGSQL_STMT_FORI)
***************
*** 822,828 ****
new = (PLpgSQL_stmt_fori *) $3;
new->label = $1;
! new->body = $4;
$$ = (PLpgSQL_stmt *) new;
}
else if ($3->cmd_type == PLPGSQL_STMT_FORS)
--- 833,839 ----
new = (PLpgSQL_stmt_fori *) $3;
new->label = $1;
! new->body = $4.stmts;
$$ = (PLpgSQL_stmt *) new;
}
else if ($3->cmd_type == PLPGSQL_STMT_FORS)
***************
*** 831,837 ****
new = (PLpgSQL_stmt_fors *) $3;
new->label = $1;
! new->body = $4;
$$ = (PLpgSQL_stmt *) new;
}
else
--- 842,848 ----
new = (PLpgSQL_stmt_fors *) $3;
new->label = $1;
! new->body = $4.stmts;
$$ = (PLpgSQL_stmt *) new;
}
else
***************
*** 841,850 ****
Assert($3->cmd_type == PLPGSQL_STMT_DYNFORS);
new = (PLpgSQL_stmt_dynfors *) $3;
new->label = $1;
! new->body = $4;
$$ = (PLpgSQL_stmt *) new;
}
/* close namespace started in opt_label */
plpgsql_ns_pop();
}
--- 852,862 ----
Assert($3->cmd_type == PLPGSQL_STMT_DYNFORS);
new = (PLpgSQL_stmt_dynfors *) $3;
new->label = $1;
! new->body = $4.stmts;
$$ = (PLpgSQL_stmt *) new;
}
+ check_labels($1, $4.end_label);
/* close namespace started in opt_label */
plpgsql_ns_pop();
}
***************
*** 1037,1043 ****
}
;
! stmt_exit : exit_type lno opt_exitlabel opt_exitcond
{
PLpgSQL_stmt_exit *new;
--- 1049,1055 ----
}
;
! stmt_exit : exit_type lno opt_label opt_exitcond
{
PLpgSQL_stmt_exit *new;
***************
*** 1245,1252 ****
}
;
! loop_body : proc_sect K_END K_LOOP ';'
! { $$ = $1; }
;
stmt_execsql : execsql_start lno
--- 1257,1267 ----
}
;
! loop_body : proc_sect K_END K_LOOP opt_label ';'
! {
! $$.stmts = $1;
! $$.end_label = $4;
! }
;
stmt_execsql : execsql_start lno
***************
*** 1262,1268 ****
}
;
! stmt_dynexecute : K_EXECUTE lno
{
PLpgSQL_stmt_dynexecute *new;
PLpgSQL_expr *expr;
--- 1277,1283 ----
}
;
! stmt_dynexecute : K_EXECUTE lno
{
PLpgSQL_stmt_dynexecute *new;
PLpgSQL_expr *expr;
***************
*** 1418,1424 ****
errmsg("cursor \"%s\" has no arguments",
$3->refname)));
}
!
if (tok != ';')
{
plpgsql_error_lineno = plpgsql_scanner_lineno();
--- 1433,1439 ----
errmsg("cursor \"%s\" has no arguments",
$3->refname)));
}
!
if (tok != ';')
{
plpgsql_error_lineno = plpgsql_scanner_lineno();
***************
*** 1596,1602 ****
{ $$ = plpgsql_read_expression(K_LOOP, "LOOP"); }
;
! opt_label :
{
plpgsql_ns_push(NULL);
$$ = NULL;
--- 1611,1617 ----
{ $$ = plpgsql_read_expression(K_LOOP, "LOOP"); }
;
! opt_block_label :
{
plpgsql_ns_push(NULL);
$$ = NULL;
***************
*** 1608,1621 ****
}
;
! opt_exitlabel :
! { $$ = NULL; }
| T_LABEL
{
! char *name;
!
! plpgsql_convert_ident(yytext, &name, 1);
! $$ = name;
}
| T_WORD
{
--- 1623,1637 ----
}
;
! opt_label :
! {
! $$ = NULL;
! }
| T_LABEL
{
! char *label_name;
! plpgsql_convert_ident(yytext, &label_name, 1);
! $$ = label_name;
}
| T_WORD
{
***************
*** 2210,2213 ****
--- 2226,2254 ----
errposition(0);
}
+ static void
+ check_labels(const char *start_label, const char *end_label)
+ {
+ if (end_label)
+ {
+ if (!start_label)
+ {
+ plpgsql_error_lineno = plpgsql_scanner_lineno();
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("end label \"%s\" specified for unlabelled block",
+ end_label)));
+ }
+
+ if (strcmp(start_label, end_label) != 0)
+ {
+ plpgsql_error_lineno = plpgsql_scanner_lineno();
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("end label \"%s\" differs from block's label \"%s\"",
+ end_label, start_label)));
+ }
+ }
+ }
+
#include "pl_scan.c"
Index: src/test/regress/expected/plpgsql.out
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/src/test/regress/expected/plpgsql.out,v
retrieving revision 1.36
diff -c -r1.36 plpgsql.out
*** src/test/regress/expected/plpgsql.out 22 Jun 2005 07:28:47 -0000 1.36
--- src/test/regress/expected/plpgsql.out 1 Jul 2005 11:53:55 -0000
***************
*** 2491,2497 ****
(1 row)
drop function raise_exprs();
! -- continue statement
create table conttesttbl(idx serial, v integer);
NOTICE: CREATE TABLE will create implicit sequence "conttesttbl_idx_seq" for serial column "conttesttbl.idx"
insert into conttesttbl(v) values(10);
--- 2491,2497 ----
(1 row)
drop function raise_exprs();
! -- continue statement
create table conttesttbl(idx serial, v integer);
NOTICE: CREATE TABLE will create implicit sequence "conttesttbl_idx_seq" for serial column "conttesttbl.idx"
insert into conttesttbl(v) values(10);
***************
*** 2532,2538 ****
for _i in 1..10 loop
begin
-- applies to outer loop, not the nested begin block
! continue when _i < 5;
raise notice '%', _i;
end;
end loop;
--- 2532,2538 ----
for _i in 1..10 loop
begin
-- applies to outer loop, not the nested begin block
! continue when _i < 5;
raise notice '%', _i;
end;
end loop;
***************
*** 2666,2668 ****
--- 2666,2723 ----
drop function continue_test2();
drop function continue_test3();
drop table conttesttbl;
+ -- verbose end block and end loop
+ create function end_label1() returns void as $$
+ <<blbl>>
+ begin
+ <<flbl1>>
+ for _i in 1 .. 10 loop
+ exit flbl1;
+ end loop flbl1;
+ <<flbl2>>
+ for _i in 1 .. 10 loop
+ exit flbl2;
+ end loop;
+ end blbl;
+ $$ language plpgsql;
+ select end_label1();
+ end_label1
+ ------------
+
+ (1 row)
+
+ drop function end_label1();
+ -- should fail: undefined end label
+ create function end_label2() returns void as $$
+ begin
+ for _i in 1 .. 10 loop
+ exit;
+ end loop flbl1;
+ end;
+ $$ language plpgsql;
+ ERROR: no such label at or near "flbl1" at character 101
+ LINE 5: end loop flbl1;
+ ^
+ -- should fail: end label does not match start label
+ create function end_label3() returns void as $$
+ <<outer_label>>
+ begin
+ <<inner_label>>
+ for _i in 1 .. 10 loop
+ exit;
+ end loop outer_label;
+ end;
+ $$ language plpgsql;
+ ERROR: end label "outer_label" differs from block's label "inner_label"
+ CONTEXT: compile of PL/pgSQL function "end_label3" near line 6
+ -- should fail: end label on a block without a start label
+ create function end_label4() returns void as $$
+ <<outer_label>>
+ begin
+ for _i in 1 .. 10 loop
+ exit;
+ end loop outer_label;
+ end;
+ $$ language plpgsql;
+ ERROR: end label "outer_label" specified for unlabelled block
+ CONTEXT: compile of PL/pgSQL function "end_label4" near line 5
Index: src/test/regress/sql/plpgsql.sql
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/src/test/regress/sql/plpgsql.sql,v
retrieving revision 1.31
diff -c -r1.31 plpgsql.sql
*** src/test/regress/sql/plpgsql.sql 22 Jun 2005 07:28:47 -0000 1.31
--- src/test/regress/sql/plpgsql.sql 1 Jul 2005 11:43:36 -0000
***************
*** 2113,2119 ****
select raise_exprs();
drop function raise_exprs();
! -- continue statement
create table conttesttbl(idx serial, v integer);
insert into conttesttbl(v) values(10);
insert into conttesttbl(v) values(20);
--- 2113,2119 ----
select raise_exprs();
drop function raise_exprs();
! -- continue statement
create table conttesttbl(idx serial, v integer);
insert into conttesttbl(v) values(10);
insert into conttesttbl(v) values(20);
***************
*** 2154,2160 ****
for _i in 1..10 loop
begin
-- applies to outer loop, not the nested begin block
! continue when _i < 5;
raise notice '%', _i;
end;
end loop;
--- 2154,2160 ----
for _i in 1..10 loop
begin
-- applies to outer loop, not the nested begin block
! continue when _i < 5;
raise notice '%', _i;
end;
end loop;
***************
*** 2232,2234 ****
--- 2232,2282 ----
drop function continue_test2();
drop function continue_test3();
drop table conttesttbl;
+
+ -- verbose end block and end loop
+ create function end_label1() returns void as $$
+ <<blbl>>
+ begin
+ <<flbl1>>
+ for _i in 1 .. 10 loop
+ exit flbl1;
+ end loop flbl1;
+ <<flbl2>>
+ for _i in 1 .. 10 loop
+ exit flbl2;
+ end loop;
+ end blbl;
+ $$ language plpgsql;
+
+ select end_label1();
+ drop function end_label1();
+
+ -- should fail: undefined end label
+ create function end_label2() returns void as $$
+ begin
+ for _i in 1 .. 10 loop
+ exit;
+ end loop flbl1;
+ end;
+ $$ language plpgsql;
+
+ -- should fail: end label does not match start label
+ create function end_label3() returns void as $$
+ <<outer_label>>
+ begin
+ <<inner_label>>
+ for _i in 1 .. 10 loop
+ exit;
+ end loop outer_label;
+ end;
+ $$ language plpgsql;
+
+ -- should fail: end label on a block without a start label
+ create function end_label4() returns void as $$
+ <<outer_label>>
+ begin
+ for _i in 1 .. 10 loop
+ exit;
+ end loop outer_label;
+ end;
+ $$ language plpgsql;
pgsql-hackers by date: