Greetings, * Pavel Stehule (pavel.steh...@gmail.com) wrote: > This patch remove redundant rows from PL/pgSQL executor (-89 lines).
While I can certainly appreciate wanting to remove redundant code, I
don't think this change actually improves the situation. The problem is
more than just that we might want to make a change to 'while' but not
'for', it's also that it makes it very difficult to follow the code
flow.
If you could have found a way to make it work for all calls to
exec_stmts(), it might be a bit better, but you can't without kludging
it up further. If you could, then it might be possible to move some of
this logic *into* exec_stmts(), but I don't see that being reasonable
either.
In the end, I'd recommend cleaning up the handling of the exec_stmts()
return code so that all of these pieces follow the same style and look
similar (I'd go with the switch-based approach and remove the if/else
branches). That'll make it easier for anyone coming along later who
does end up needing to change all three.
> Doesn't change a functionality.
I'm not convinced of this either, to be honest.. In exec_stmt_while(),
there is:
for(;;)
{
[...]
if (isnull || !value)
break;
rc = exec_stmts(estate, stmt->body);
[...]
}
return PLPGSQL_RC_OK;
You replaced the last return with:
return rc;
Which could mean returning an uninitialized rc if the above break;
happens.
In the end, I guess it's up to the committers on how they feel about
this, so here's an updated patch which fixes the above issue and
improves the comments/grammer. Passes all regressions and works in my
limited testing.
commit e6639d83db5b301e184bf158b1591007a3f79526
Author: Stephen Frost <sfr...@snowman.net>
Date: Wed Jan 19 14:28:36 2011 -0500
Improve comments in pl_exec.c wrt can_leave_loop()
This patch improves some of the comments around can_leave_loop(), and
fixes a couple of fall-through cases to make sure they behave the same
as before the code de-duplication patch that introduced
can_leave_loop().
commit f8262adcba662164dbc24d289cb036b3f0aee582
Author: Stephen Frost <sfr...@snowman.net>
Date: Wed Jan 19 14:26:27 2011 -0500
remove redundant code from pl_exec.c
This patch removes redundant handling of exec_stmts()'s return code
by creating a new function to be called after exec_stmts() is run.
This new function will then handle the return code, update it if
necessary, and return if the loop should continue or not.
Patch by Pavel Stehule.
Thanks,
Stephen
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
***************
*** 204,209 **** static Portal exec_dynquery_with_params(PLpgSQL_execstate *estate,
--- 204,211 ----
PLpgSQL_expr *dynquery, List *params,
const char *portalname, int cursorOptions);
+ static bool can_leave_loop(PLpgSQL_execstate *estate,
+ PLpgSQL_any_loop *stmt, int *rc);
/* ----------
* plpgsql_exec_function Called by the call handler for
***************
*** 1566,1571 **** exec_stmt_case(PLpgSQL_execstate *estate, PLpgSQL_stmt_case *stmt)
--- 1568,1650 ----
return exec_stmts(estate, stmt->else_stmts);
}
+ /*
+ * can_leave_loop
+ *
+ * Check the result of exec_stmts for the various exec_stmt_loop
+ * functions (unconditional loop, while loop, for-over-integer loop,
+ * for-over-portal loop).
+ *
+ * Returns true when the outer cycle should be left, otherwise false.
+ * Will also update the return code (rc) as necessary.
+ */
+ static bool
+ can_leave_loop(PLpgSQL_execstate *estate, PLpgSQL_any_loop *stmt, int *rc)
+ {
+ /*
+ * Check which return code exec_stmts() returned and handle it
+ * accordingly.
+ */
+ switch (*rc)
+ {
+ case PLPGSQL_RC_OK:
+ /* Just continue the outer loop on PLPGSQL_RC_OK */
+ return false;
+
+ case PLPGSQL_RC_RETURN:
+ /* Time to break out of the outer loop */
+ return true;
+
+ case PLPGSQL_RC_EXIT:
+ if (estate->exitlabel == NULL)
+ {
+ /* unlabelled exit, finish the current loop */
+ *rc = PLPGSQL_RC_OK;
+ }
+ if (stmt->label != NULL && strcmp(stmt->label, estate->exitlabel) == 0)
+ {
+ /* labelled exit, matches the current stmt's label */
+ estate->exitlabel = NULL;
+ *rc = PLPGSQL_RC_OK;
+ }
+
+ /*
+ * otherwise, this is a labelled exit that does not match the
+ * current statement's label, if any: return RC_EXIT so that the
+ * EXIT continues to propagate up the stack.
+ */
+ return true;
+
+ case PLPGSQL_RC_CONTINUE:
+ if (estate->exitlabel == NULL)
+ {
+ /* anonymous continue, so re-run the loop */
+ *rc = PLPGSQL_RC_OK;
+ }
+ else if (stmt->label != NULL &&
+ strcmp(stmt->label, estate->exitlabel) == 0)
+ {
+ /* label matches named continue, so re-run loop */
+ estate->exitlabel = NULL;
+ *rc = PLPGSQL_RC_OK;
+ }
+ else
+ {
+ /*
+ * otherwise, this is a named continue that does not match the
+ * current statement's label, if any: return RC_CONTINUE so
+ * that the CONTINUE will propagate up the stack.
+ */
+ return true;
+ }
+
+ return false;
+
+ default:
+ elog(ERROR, "unrecognized rc: %d", *rc);
+ return false; /* keep compiler quiet */
+ }
+ }
/* ----------
* exec_stmt_loop Loop over statements until
***************
*** 1575,1620 **** exec_stmt_case(PLpgSQL_execstate *estate, PLpgSQL_stmt_case *stmt)
static int
exec_stmt_loop(PLpgSQL_execstate *estate, PLpgSQL_stmt_loop *stmt)
{
for (;;)
{
! int rc = exec_stmts(estate, stmt->body);
!
! switch (rc)
! {
! case PLPGSQL_RC_OK:
! break;
!
! case PLPGSQL_RC_EXIT:
! if (estate->exitlabel == NULL)
! return PLPGSQL_RC_OK;
! if (stmt->label == NULL)
! return PLPGSQL_RC_EXIT;
! if (strcmp(stmt->label, estate->exitlabel) != 0)
! return PLPGSQL_RC_EXIT;
! estate->exitlabel = NULL;
! return PLPGSQL_RC_OK;
!
! case PLPGSQL_RC_CONTINUE:
! if (estate->exitlabel == NULL)
! /* anonymous continue, so re-run the loop */
! break;
! else if (stmt->label != NULL &&
! strcmp(stmt->label, estate->exitlabel) == 0)
! /* label matches named continue, so re-run loop */
! estate->exitlabel = NULL;
! else
! /* label doesn't match named continue, so propagate upward */
! return PLPGSQL_RC_CONTINUE;
! break;
!
! case PLPGSQL_RC_RETURN:
! return rc;
! default:
! elog(ERROR, "unrecognized rc: %d", rc);
! }
}
return PLPGSQL_RC_OK;
}
--- 1654,1670 ----
static int
exec_stmt_loop(PLpgSQL_execstate *estate, PLpgSQL_stmt_loop *stmt)
{
+ int rc;
+
for (;;)
{
! rc = exec_stmts(estate, stmt->body);
! if (can_leave_loop(estate, (PLpgSQL_any_loop *) stmt, &rc))
! return rc;
}
+ /* Keep compiler happy, but we should never get here */
return PLPGSQL_RC_OK;
}
***************
*** 1628,1636 **** exec_stmt_loop(PLpgSQL_execstate *estate, PLpgSQL_stmt_loop *stmt)
static int
exec_stmt_while(PLpgSQL_execstate *estate, PLpgSQL_stmt_while *stmt)
{
for (;;)
{
- int rc;
bool value;
bool isnull;
--- 1678,1687 ----
static int
exec_stmt_while(PLpgSQL_execstate *estate, PLpgSQL_stmt_while *stmt)
{
+ int rc;
+
for (;;)
{
bool value;
bool isnull;
***************
*** 1642,1683 **** exec_stmt_while(PLpgSQL_execstate *estate, PLpgSQL_stmt_while *stmt)
rc = exec_stmts(estate, stmt->body);
! switch (rc)
! {
! case PLPGSQL_RC_OK:
! break;
!
! case PLPGSQL_RC_EXIT:
! if (estate->exitlabel == NULL)
! return PLPGSQL_RC_OK;
! if (stmt->label == NULL)
! return PLPGSQL_RC_EXIT;
! if (strcmp(stmt->label, estate->exitlabel) != 0)
! return PLPGSQL_RC_EXIT;
! estate->exitlabel = NULL;
! return PLPGSQL_RC_OK;
!
! case PLPGSQL_RC_CONTINUE:
! if (estate->exitlabel == NULL)
! /* anonymous continue, so re-run loop */
! break;
! else if (stmt->label != NULL &&
! strcmp(stmt->label, estate->exitlabel) == 0)
! /* label matches named continue, so re-run loop */
! estate->exitlabel = NULL;
! else
! /* label doesn't match named continue, propagate upward */
! return PLPGSQL_RC_CONTINUE;
! break;
!
! case PLPGSQL_RC_RETURN:
! return rc;
!
! default:
! elog(ERROR, "unrecognized rc: %d", rc);
! }
}
return PLPGSQL_RC_OK;
}
--- 1693,1703 ----
rc = exec_stmts(estate, stmt->body);
! if (can_leave_loop(estate, (PLpgSQL_any_loop *) stmt, &rc))
! return rc;
}
+ /* In case we fall through from above */
return PLPGSQL_RC_OK;
}
***************
*** 1789,1838 **** exec_stmt_fori(PLpgSQL_execstate *estate, PLpgSQL_stmt_fori *stmt)
*/
rc = exec_stmts(estate, stmt->body);
! if (rc == PLPGSQL_RC_RETURN)
! break; /* break out of the loop */
! else if (rc == PLPGSQL_RC_EXIT)
! {
! if (estate->exitlabel == NULL)
! /* unlabelled exit, finish the current loop */
! rc = PLPGSQL_RC_OK;
! else if (stmt->label != NULL &&
! strcmp(stmt->label, estate->exitlabel) == 0)
! {
! /* labelled exit, matches the current stmt's label */
! estate->exitlabel = NULL;
! rc = PLPGSQL_RC_OK;
! }
!
! /*
! * otherwise, this is a labelled exit that does not match the
! * 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)
- /* unlabelled continue, so re-run the current loop */
- rc = PLPGSQL_RC_OK;
- else if (stmt->label != NULL &&
- strcmp(stmt->label, estate->exitlabel) == 0)
- {
- /* label matches named continue, so re-run loop */
- estate->exitlabel = NULL;
- rc = PLPGSQL_RC_OK;
- }
- else
- {
- /*
- * otherwise, this is a named continue that does not match the
- * current statement's label, if any: return RC_CONTINUE so
- * that the CONTINUE will propagate up the stack.
- */
- break;
- }
- }
/*
* Increase/decrease loop value, unless it would overflow, in which
--- 1809,1816 ----
*/
rc = exec_stmts(estate, stmt->body);
! if (can_leave_loop(estate, (PLpgSQL_any_loop *) stmt, &rc))
break;
/*
* Increase/decrease loop value, unless it would overflow, in which
***************
*** 4410,4469 **** exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
*/
rc = exec_stmts(estate, stmt->body);
! if (rc != PLPGSQL_RC_OK)
! {
! if (rc == PLPGSQL_RC_EXIT)
! {
! if (estate->exitlabel == NULL)
! {
! /* unlabelled exit, so exit the current loop */
! rc = PLPGSQL_RC_OK;
! }
! else if (stmt->label != NULL &&
! strcmp(stmt->label, estate->exitlabel) == 0)
! {
! /* label matches this loop, so exit loop */
! estate->exitlabel = NULL;
! rc = PLPGSQL_RC_OK;
! }
!
! /*
! * otherwise, we processed a labelled exit that does not
! * match the current statement's label, if any; return
! * RC_EXIT so that the EXIT continues to recurse upward.
! */
! }
! else if (rc == PLPGSQL_RC_CONTINUE)
! {
! if (estate->exitlabel == NULL)
! {
! /* unlabelled continue, so re-run the current loop */
! rc = PLPGSQL_RC_OK;
! continue;
! }
! else if (stmt->label != NULL &&
! strcmp(stmt->label, estate->exitlabel) == 0)
! {
! /* label matches this loop, so re-run loop */
! estate->exitlabel = NULL;
! rc = PLPGSQL_RC_OK;
! continue;
! }
!
! /*
! * otherwise, we process a labelled continue that does not
! * match the current statement's label, if any; return
! * RC_CONTINUE so that the CONTINUE will propagate up the
! * stack.
! */
! }
!
! /*
! * We're aborting the loop. Need a goto to get out of two
! * levels of loop...
! */
goto loop_exit;
- }
}
SPI_freetuptable(tuptab);
--- 4388,4395 ----
*/
rc = exec_stmts(estate, stmt->body);
! if (can_leave_loop(estate, (PLpgSQL_any_loop *) stmt, &rc))
goto loop_exit;
}
SPI_freetuptable(tuptab);
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
***************
*** 407,412 **** typedef struct /* one arm of CASE statement */
--- 407,420 ----
} PLpgSQL_case_when;
+ typedef struct /* Ancestor of loops */
+ {
+ int cmd_type;
+ int lineno;
+ char *label;
+ } PLpgSQL_any_loop;
+
+
typedef struct
{ /* Unconditional LOOP statement */
int cmd_type;
signature.asc
Description: Digital signature
