On 8/17/15 4:46 PM, Tom Lane wrote:
Jim Nasby <jim.na...@bluetreble.com> writes:
On 8/17/15 9:48 AM, Tom Lane wrote:
I'm inclined to think that if we wanted to make this better, the way to
improve it would be to detect the error*at compile time*, and get rid of
this hack in plpgsql_exec_function altogether.
So split PLPGSQL_NSTYPE_LABEL into PLPGSQL_NSTYPE_BLOCK_LABEL and
PLPGSQL_NSTYPE_LOOP_LABEL, and split opt_block_label and opt_label the
same way?
I think using two NSTYPE codes would probably be a pain because there are
numerous places that don't care about the distinction; it'd be better to
have a secondary attribute distinguishing these cases. (It looks like you
could perhaps reuse the "itemno" field for the purpose, since that seems
to be going unused in LABEL items.)
You likely do need to split opt_block_label into two productions, since
that will be the easiest way to pass forward the knowledge of whether
it's being called from a loop or non-loop construct.
Here's a patch that does that. This also made it possible to check for
CONTINUE/EXIT being used outside a loop during parsing, so I changed
that as well and removed those checks from pl_exec.c. I refactored the 3
places that were doing the check into exec_stmt_block(), renaming the
original function exec_stmt_block_rc for the one place that still needs
the return code.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 05268e3..1ae4bb7 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -371,7 +371,7 @@ do_compile(FunctionCallInfo fcinfo,
* variables (such as FOUND), and is named after the function itself.
*/
plpgsql_ns_init();
- plpgsql_ns_push(NameStr(procStruct->proname));
+ plpgsql_ns_push(NameStr(procStruct->proname), PLPGSQL_LABEL_BLOCK);
plpgsql_DumpExecTree = false;
plpgsql_start_datums();
@@ -851,7 +851,7 @@ plpgsql_compile_inline(char *proc_source)
function->extra_errors = 0;
plpgsql_ns_init();
- plpgsql_ns_push(func_name);
+ plpgsql_ns_push(func_name, PLPGSQL_LABEL_BLOCK);
plpgsql_DumpExecTree = false;
plpgsql_start_datums();
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index fb93336..c953e3c 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -131,7 +131,9 @@ static HTAB *shared_cast_hash = NULL;
static void plpgsql_exec_error_callback(void *arg);
static PLpgSQL_datum *copy_plpgsql_datum(PLpgSQL_datum *datum);
-static int exec_stmt_block(PLpgSQL_execstate *estate,
+static void exec_stmt_block(PLpgSQL_execstate *estate,
+ PLpgSQL_stmt_block *block);
+static int exec_stmt_block_rc(PLpgSQL_execstate *estate,
PLpgSQL_stmt_block *block);
static int exec_stmts(PLpgSQL_execstate *estate,
List *stmts);
@@ -303,7 +305,6 @@ plpgsql_exec_function(PLpgSQL_function *func,
FunctionCallInfo fcinfo,
PLpgSQL_execstate estate;
ErrorContextCallback plerrcontext;
int i;
- int rc;
/*
* Setup the execution state
@@ -432,25 +433,7 @@ plpgsql_exec_function(PLpgSQL_function *func,
FunctionCallInfo fcinfo,
*/
estate.err_text = NULL;
estate.err_stmt = (PLpgSQL_stmt *) (func->action);
- rc = exec_stmt_block(&estate, func->action);
- if (rc != PLPGSQL_RC_RETURN)
- {
- estate.err_stmt = NULL;
- estate.err_text = NULL;
-
- /*
- * Provide a more helpful message if a CONTINUE has been used
outside
- * the context it can work in.
- */
- if (rc == PLPGSQL_RC_CONTINUE)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("CONTINUE cannot be used
outside a loop")));
- else
- ereport(ERROR,
-
(errcode(ERRCODE_S_R_E_FUNCTION_EXECUTED_NO_RETURN_STATEMENT),
- errmsg("control reached end of function without
RETURN")));
- }
+ exec_stmt_block(&estate, func->action);
/*
* We got a return value - process it
@@ -598,7 +581,6 @@ plpgsql_exec_trigger(PLpgSQL_function *func,
PLpgSQL_execstate estate;
ErrorContextCallback plerrcontext;
int i;
- int rc;
PLpgSQL_var *var;
PLpgSQL_rec *rec_new,
*rec_old;
@@ -786,25 +768,7 @@ plpgsql_exec_trigger(PLpgSQL_function *func,
*/
estate.err_text = NULL;
estate.err_stmt = (PLpgSQL_stmt *) (func->action);
- rc = exec_stmt_block(&estate, func->action);
- if (rc != PLPGSQL_RC_RETURN)
- {
- estate.err_stmt = NULL;
- estate.err_text = NULL;
-
- /*
- * Provide a more helpful message if a CONTINUE has been used
outside
- * the context it can work in.
- */
- if (rc == PLPGSQL_RC_CONTINUE)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("CONTINUE cannot be used
outside a loop")));
- else
- ereport(ERROR,
-
(errcode(ERRCODE_S_R_E_FUNCTION_EXECUTED_NO_RETURN_STATEMENT),
- errmsg("control reached end of trigger
procedure without RETURN")));
- }
+ exec_stmt_block(&estate, func->action);
estate.err_stmt = NULL;
estate.err_text = gettext_noop("during function exit");
@@ -871,7 +835,6 @@ plpgsql_exec_event_trigger(PLpgSQL_function *func,
EventTriggerData *trigdata)
PLpgSQL_execstate estate;
ErrorContextCallback plerrcontext;
int i;
- int rc;
PLpgSQL_var *var;
/*
@@ -914,25 +877,7 @@ plpgsql_exec_event_trigger(PLpgSQL_function *func,
EventTriggerData *trigdata)
*/
estate.err_text = NULL;
estate.err_stmt = (PLpgSQL_stmt *) (func->action);
- rc = exec_stmt_block(&estate, func->action);
- if (rc != PLPGSQL_RC_RETURN)
- {
- estate.err_stmt = NULL;
- estate.err_text = NULL;
-
- /*
- * Provide a more helpful message if a CONTINUE has been used
outside
- * the context it can work in.
- */
- if (rc == PLPGSQL_RC_CONTINUE)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("CONTINUE cannot be used
outside a loop")));
- else
- ereport(ERROR,
-
(errcode(ERRCODE_S_R_E_FUNCTION_EXECUTED_NO_RETURN_STATEMENT),
- errmsg("control reached end of trigger
procedure without RETURN")));
- }
+ exec_stmt_block(&estate, func->action);
estate.err_stmt = NULL;
estate.err_text = gettext_noop("during function exit");
@@ -1108,11 +1053,11 @@ exception_matches_conditions(ErrorData *edata,
PLpgSQL_condition *cond)
/* ----------
- * exec_stmt_block Execute a block of statements
+ * exec_stmt_block_rc Execute a block of statements
* ----------
*/
static int
-exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
+exec_stmt_block_rc(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
{
volatile int rc = -1;
int i;
@@ -1463,7 +1408,7 @@ exec_stmt(PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt)
switch ((enum PLpgSQL_stmt_types) stmt->cmd_type)
{
case PLPGSQL_STMT_BLOCK:
- rc = exec_stmt_block(estate, (PLpgSQL_stmt_block *)
stmt);
+ rc = exec_stmt_block_rc(estate, (PLpgSQL_stmt_block *)
stmt);
break;
case PLPGSQL_STMT_ASSIGN:
@@ -7201,6 +7146,27 @@ exec_dynquery_with_params(PLpgSQL_execstate *estate,
return portal;
}
+/* ----------
+ * exec_stmt_block Execute block of statements and
sanity-check return code
+ * ----------
+ */
+static void exec_stmt_block(PLpgSQL_execstate *estate,
+ PLpgSQL_stmt_block *block)
+{
+ int rc = exec_stmt_block_rc(estate, block);
+
+ if (rc != PLPGSQL_RC_RETURN)
+ {
+ estate->err_stmt = NULL;
+ estate->err_text = NULL;
+
+ Assert(rc == PLPGSQL_RC_OK);
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_FUNCTION_EXECUTED_NO_RETURN_STATEMENT),
+ errmsg("control reached end of function without RETURN")));
+ }
+}
+
/*
* Return a formatted string with information about an expression's parameters,
* or NULL if the expression does not take any parameters.
diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index 900ba3c..9269b51 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -51,11 +51,11 @@ plpgsql_ns_init(void)
* ----------
*/
void
-plpgsql_ns_push(const char *label)
+plpgsql_ns_push(const char *label, enum PLpgSQL_label_types label_type)
{
if (label == NULL)
label = "";
- plpgsql_ns_additem(PLPGSQL_NSTYPE_LABEL, 0, label);
+ plpgsql_ns_additem(PLPGSQL_NSTYPE_LABEL, label_type, label);
}
@@ -206,6 +206,25 @@ plpgsql_ns_lookup_label(PLpgSQL_nsitem *ns_cur, const char
*name)
}
+/* ----------
+ * plpgsql_ns_find_loop Lookup a loop label in the
given namespace chain
+ * ----------
+ */
+PLpgSQL_nsitem *
+plpgsql_ns_find_loop(PLpgSQL_nsitem *ns_cur)
+{
+ while (ns_cur != NULL)
+ {
+ if (ns_cur->itemtype == PLPGSQL_NSTYPE_LABEL &&
+ ns_cur->itemno == PLPGSQL_LABEL_LOOP)
+ return ns_cur;
+ ns_cur = ns_cur->prev;
+ }
+
+ return NULL; /* label not found */
+}
+
+
/*
* Statement type as a string, for use in error messages etc.
*/
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 0097890..54fc207 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -186,7 +186,7 @@ static void
check_raise_parameters(PLpgSQL_stmt_raise *stmt);
%type <forvariable> for_variable
%type <stmt> for_control
-%type <str> any_identifier opt_block_label opt_label option_value
+%type <str> any_identifier opt_block_label opt_loop_label opt_label
option_value
%type <list> proc_sect stmt_elsifs stmt_else
%type <loop_body> loop_body
@@ -535,7 +535,7 @@ decl_statement : decl_varname decl_const decl_datatype
decl_collate decl_notnull
$4->itemno, $1.name);
}
| decl_varname opt_scrollable K_CURSOR
- { plpgsql_ns_push($1.name); }
+ { plpgsql_ns_push($1.name,
PLPGSQL_LABEL_OTHER); }
decl_cursor_args decl_is_for decl_cursor_query
{
PLpgSQL_var *new;
@@ -1218,7 +1218,7 @@ opt_case_else :
}
;
-stmt_loop : opt_block_label K_LOOP loop_body
+stmt_loop : opt_loop_label K_LOOP loop_body
{
PLpgSQL_stmt_loop *new;
@@ -1235,7 +1235,7 @@ stmt_loop : opt_block_label K_LOOP loop_body
}
;
-stmt_while : opt_block_label K_WHILE expr_until_loop loop_body
+stmt_while : opt_loop_label K_WHILE expr_until_loop loop_body
{
PLpgSQL_stmt_while *new;
@@ -1253,7 +1253,7 @@ stmt_while : opt_block_label K_WHILE
expr_until_loop loop_body
}
;
-stmt_for : opt_block_label K_FOR for_control loop_body
+stmt_for : opt_loop_label K_FOR for_control loop_body
{
/* This runs after we've
scanned the loop body */
if ($3->cmd_type ==
PLPGSQL_STMT_FORI)
@@ -1282,7 +1282,7 @@ stmt_for : opt_block_label K_FOR for_control
loop_body
}
check_labels($1, $4.end_label,
$4.end_label_location);
- /* close namespace started in
opt_block_label */
+ /* close namespace started in
opt_loop_label */
plpgsql_ns_pop();
}
;
@@ -1603,7 +1603,7 @@ for_variable : T_DATUM
}
;
-stmt_foreach_a : opt_block_label K_FOREACH for_variable foreach_slice K_IN
K_ARRAY expr_until_loop loop_body
+stmt_foreach_a : opt_loop_label K_FOREACH for_variable foreach_slice K_IN
K_ARRAY expr_until_loop loop_body
{
PLpgSQL_stmt_foreach_a *new;
@@ -1666,6 +1666,28 @@ stmt_exit : exit_type opt_label
opt_exitcond
new->label = $2;
new->cond = $3;
+ /* If we have a label make sure
it's a loop label */
+ if ($2)
+ {
+ PLpgSQL_nsitem *label;
+ label =
plpgsql_ns_lookup_label(plpgsql_ns_top(), $2);
+
+ if (label->itemno !=
PLPGSQL_LABEL_LOOP)
+ ereport(ERROR,
+
(errcode(ERRCODE_SYNTAX_ERROR),
+
errmsg("block label used in %s",
+
plpgsql_stmt_typename((PLpgSQL_stmt *)new)),
+
parser_errposition(@2)));
+
+ }
+ else
+ if (! plpgsql_ns_find_loop(plpgsql_ns_top()))
+ ereport(ERROR,
+
(errcode(ERRCODE_SYNTAX_ERROR),
+
errmsg("%s cannot be used outside a loop",
+
plpgsql_stmt_typename((PLpgSQL_stmt *)new)),
+
parser_errposition(@1)));
+
$$ = (PLpgSQL_stmt *)new;
}
;
@@ -2290,12 +2312,24 @@ expr_until_loop :
opt_block_label :
{
- plpgsql_ns_push(NULL);
+ plpgsql_ns_push(NULL,
PLPGSQL_LABEL_BLOCK);
+ $$ = NULL;
+ }
+ | LESS_LESS any_identifier GREATER_GREATER
+ {
+ plpgsql_ns_push($2,
PLPGSQL_LABEL_BLOCK);
+ $$ = $2;
+ }
+ ;
+
+opt_loop_label :
+ {
+ plpgsql_ns_push(NULL,
PLPGSQL_LABEL_LOOP);
$$ = NULL;
}
| LESS_LESS any_identifier GREATER_GREATER
{
- plpgsql_ns_push($2);
+ plpgsql_ns_push($2,
PLPGSQL_LABEL_LOOP);
$$ = $2;
}
;
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index fd5679c..5d2ee99 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -47,6 +47,17 @@ enum
};
/* ----------
+ * Types of labels for PLPGSQL_NSTYPE_LABEL
+ * ----------
+ */
+enum PLpgSQL_label_types
+{
+ PLPGSQL_LABEL_OTHER,
+ PLPGSQL_LABEL_BLOCK,
+ PLPGSQL_LABEL_LOOP
+};
+
+/* ----------
* Datum array node types
* ----------
*/
@@ -330,7 +341,7 @@ typedef struct
typedef struct PLpgSQL_nsitem
{ /* Item in the
compilers namespace tree */
int itemtype;
- int itemno;
+ int itemno; /* For labels this is
PLpgSQL_label_types */
struct PLpgSQL_nsitem *prev;
char name[FLEXIBLE_ARRAY_MEMBER]; /* nul-terminated
string */
} PLpgSQL_nsitem;
@@ -997,7 +1008,7 @@ extern void exec_get_datum_type_info(PLpgSQL_execstate
*estate,
* ----------
*/
extern void plpgsql_ns_init(void);
-extern void plpgsql_ns_push(const char *label);
+extern void plpgsql_ns_push(const char *label, enum PLpgSQL_label_types
label_type);
extern void plpgsql_ns_pop(void);
extern PLpgSQL_nsitem *plpgsql_ns_top(void);
extern void plpgsql_ns_additem(int itemtype, int itemno, const char *name);
@@ -1006,6 +1017,7 @@ extern PLpgSQL_nsitem *plpgsql_ns_lookup(PLpgSQL_nsitem
*ns_cur, bool localmode,
const char *name3, int *names_used);
extern PLpgSQL_nsitem *plpgsql_ns_lookup_label(PLpgSQL_nsitem *ns_cur,
const char *name);
+extern PLpgSQL_nsitem *plpgsql_ns_find_loop(PLpgSQL_nsitem *ns_cur);
/* ----------
* Other functions in pl_funcs.c
diff --git a/src/test/regress/expected/plpgsql.out
b/src/test/regress/expected/plpgsql.out
index 452ef9d..d9e0f2d 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -2830,7 +2830,9 @@ NOTICE: 10
(1 row)
--- CONTINUE is only legal inside a loop
+drop function continue_test1();
+drop table conttesttbl;
+-- should fail: CONTINUE is only legal inside a loop
create function continue_test2() returns void as $$
begin
begin
@@ -2839,11 +2841,22 @@ begin
return;
end;
$$ language plpgsql;
--- should fail
-select continue_test2();
ERROR: CONTINUE cannot be used outside a loop
-CONTEXT: PL/pgSQL function continue_test2()
--- CONTINUE can't reference the label of a named block
+LINE 4: continue;
+ ^
+-- should fail: EXIT is only legal inside a loop
+create function exit_test2() returns void as $$
+begin
+ begin
+ exit;
+ end;
+ return;
+end;
+$$ language plpgsql;
+ERROR: EXIT cannot be used outside a loop
+LINE 4: exit;
+ ^
+-- should fail: CONTINUE can't reference the label of a named block
create function continue_test3() returns void as $$
begin
<<begin_block1>>
@@ -2854,14 +2867,23 @@ begin
end;
end;
$$ language plpgsql;
--- should fail
-select continue_test3();
-ERROR: CONTINUE cannot be used outside a loop
-CONTEXT: PL/pgSQL function continue_test3()
-drop function continue_test1();
-drop function continue_test2();
-drop function continue_test3();
-drop table conttesttbl;
+ERROR: block label used in CONTINUE
+LINE 6: continue begin_block1;
+ ^
+-- should fail: EXIT can't reference the label of a named block
+create function exit_test3() returns void as $$
+begin
+ <<begin_block1>>
+ begin
+ loop
+ exit begin_block1;
+ end loop;
+ end;
+end;
+$$ language plpgsql;
+ERROR: block label used in EXIT
+LINE 6: exit begin_block1;
+ ^
-- verbose end block and end loop
create function end_label1() returns void as $$
<<blbl>>
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index b46439e..4435eb7 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2360,8 +2360,10 @@ begin
end; $$ language plpgsql;
select continue_test1();
+drop function continue_test1();
+drop table conttesttbl;
--- CONTINUE is only legal inside a loop
+-- should fail: CONTINUE is only legal inside a loop
create function continue_test2() returns void as $$
begin
begin
@@ -2371,10 +2373,17 @@ begin
end;
$$ language plpgsql;
--- should fail
-select continue_test2();
+-- should fail: EXIT is only legal inside a loop
+create function exit_test2() returns void as $$
+begin
+ begin
+ exit;
+ end;
+ return;
+end;
+$$ language plpgsql;
--- CONTINUE can't reference the label of a named block
+-- should fail: CONTINUE can't reference the label of a named block
create function continue_test3() returns void as $$
begin
<<begin_block1>>
@@ -2386,13 +2395,17 @@ begin
end;
$$ language plpgsql;
--- should fail
-select continue_test3();
-
-drop function continue_test1();
-drop function continue_test2();
-drop function continue_test3();
-drop table conttesttbl;
+-- should fail: EXIT can't reference the label of a named block
+create function exit_test3() returns void as $$
+begin
+ <<begin_block1>>
+ begin
+ loop
+ exit begin_block1;
+ end loop;
+ end;
+end;
+$$ language plpgsql;
-- verbose end block and end loop
create function end_label1() returns void as $$
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
