Error message with plpgsql CONTINUE

2015-08-17 6:19 GMT+02:00 Jim Nasby <Jim.Nasby@bluetreble.com>:

Calling CONTINUE with a label that's not a loop produces an error message
with no context info [1]. This is because of

rc = exec_stmt_block(&estate, func->action);
if (rc != PLPGSQL_RC_RETURN)
{
estate.err_stmt = NULL;
estate.err_text = NULL;

I trawled through git blame a bit and it looks like it's been that way for
a very long time.

I think err_stmt should probably only be reset in the non-return case a
bit below that. I'm not sure about err_text though. Also, the code treats
PLPGSQL_RC_OK and PLPGSQL_RC_EXIT the same, which seems like a bug; I would
think PLPGSQL_RC_EXIT should be handled the same way as CONTINUE.

If someone can confirm this and tell me what to do about err_text I'll
submit a patch.

maybe "during function exit" ?

Regards

Pavel

Jim Nasby <Jim.Nasby@BlueTreble.com> writes:

Calling CONTINUE with a label that's not a loop produces an error
message with no context info [1].

True.

I think err_stmt should probably only be reset in the non-return case a
bit below that. I'm not sure about err_text though.

That is not going to help, as you'd soon find if you experimented:
given your example, the produced error message would be

ERROR: CONTINUE cannot be used outside a loop
CONTEXT: PL/pgSQL function inline_code_block line 2 at statement block

rather than pointing at the CONTINUE. To get where you needed to be,
you'd need to have some complicated and fragile rules about where err_stmt
is reset or not reset as a statement nest gets unwound.

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. pl_gram.y already
successfully detects cases where CONTINUE mentions a label that doesn't
exist or isn't surrounding the CONTINUE. What it is missing is that we
don't distinguish labels on loops from labels on non-loop statements, and
thus it can't tell if CONTINUE is referencing a non-loop label or has no
label but is not inside any loop-type statement. Seems like that detail
could be added to the PLpgSQL_nsitem data structure without a huge amount
of work.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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. pl_gram.y already
successfully detects cases where CONTINUE mentions a label that doesn't
exist or isn't surrounding the CONTINUE. What it is missing is that we
don't distinguish labels on loops from labels on non-loop statements, and
thus it can't tell if CONTINUE is referencing a non-loop label or has no
label but is not inside any loop-type statement. Seems like that detail
could be added to the PLpgSQL_nsitem data structure without a huge amount
of work.

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?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Jim Nasby <Jim.Nasby@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.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi

2015-08-17 23:46 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:

Jim Nasby <Jim.Nasby@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.

when I implemented this check in plpgsql_check I found another minor issue
in CONTINUE statement - the typename is wrong

Regards

Pavel

Pavel Stehule <pavel.stehule@gmail.com> writes:

when I implemented this check in plpgsql_check I found another minor issue
in CONTINUE statement - the typename is wrong

Hmmm ... a bit of nosing around says that fetch/move and get diagnostics
are similarly sloppy. Will fix.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8/17/15 4:46 PM, Tom Lane wrote:

Jim Nasby <Jim.Nasby@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

Jim Nasby <Jim.Nasby@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.

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.

Applied with some fixes. The major oversight was that EXIT does *not*
have the same rules as CONTINUE, as is clearly documented (though in your
defense, there was no regression test verifying the behavior ... there is
now).

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.

I did not like that part. Simpler and less code churn to just take out
the now-unnecessary outer-level tests. Also, your way lost the separate
error texts for "control reached end of function" and "control reached end
of trigger procedure", which while maybe not very important was not an
agreed-to change.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8/21/15 7:21 PM, Tom Lane wrote:

Applied with some fixes. The major oversight was that EXIT does*not*
have the same rules as CONTINUE, as is clearly documented (though in your
defense, there was no regression test verifying the behavior ... there is
now).

Yay more tests.

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.

I did not like that part. Simpler and less code churn to just take out
the now-unnecessary outer-level tests. Also, your way lost the separate
error texts for "control reached end of function" and "control reached end
of trigger procedure", which while maybe not very important was not an
agreed-to change.

Guess I didn't look hard enough at what I was removing. I was of two
minds on the refactoring anyway.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I had a few second thoughts about the wording of the error messages
in this area.

First, consider

create or replace function foo() returns void language plpgsql as $$
begin
<<lab1>>
loop
exit lab1; -- ok
end loop;
loop
exit lab1; -- not so ok
end loop;
end$$;

ERROR: label "lab1" does not exist
LINE 8: exit lab1; -- not so ok
^

This message seems confusing: label "lab1" does exist, it's just not
attached to the right loop. In a larger function that might not be too
obvious, and I can easily imagine somebody wasting some time before
figuring out the cause of his problem. Given the way the namespace data
structure works, I am not sure that we can realistically detect at line 8
that there was an instance of lab1 earlier, but perhaps we could word the
error message to cover either possibility. Maybe something like "there is
no label "foo" surrounding this statement"?

Second, consider

create or replace function foo() returns void language plpgsql as $$
begin
<<lab1>>
begin
exit lab1; -- ok
exit; -- not so ok
end;
end$$;

ERROR: EXIT cannot be used outside a loop
LINE 6: exit; -- not so ok
^

This is not too accurate, as shown by the fact that the first EXIT is
accepted. Perhaps "EXIT without a label cannot be used outside a loop"?

I realize that this is pretty nitpicky, but if we're going to all the
trouble of improving the error messages about these things, seems like
we ought to be careful about what the messages actually say.

I'm not married to these particular wordings though. Suggestions?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8/22/15 2:53 PM, Tom Lane wrote:

This message seems confusing: label "lab1" does exist, it's just not
attached to the right loop. In a larger function that might not be too
obvious, and I can easily imagine somebody wasting some time before

Agreed.

figuring out the cause of his problem. Given the way the namespace data
structure works, I am not sure that we can realistically detect at line 8
that there was an instance of lab1 earlier, but perhaps we could word the

Are there any other reasons we'd want to improve the ns stuff? Doesn't
seem worth it for just this case, but if there were other nitpicks
elsewhere maybe it is.

error message to cover either possibility. Maybe something like "there is
no label "foo" surrounding this statement"?

"surrounding" seems pretty nebulous. Maybe "no label "foo" in this
context"? I'd say we use the term block, but we differentiate between
blocks and loops. Perhaps it would be best to document namespaces and
make it clear that blocks and loops both use them. :/

Regardless of that, a hint is probably warranted. "Is "foo" a label for
an adjacent block or loop?"?

This is not too accurate, as shown by the fact that the first EXIT is
accepted. Perhaps "EXIT without a label cannot be used outside a loop"?

+1

I realize that this is pretty nitpicky, but if we're going to all the
trouble of improving the error messages about these things, seems like
we ought to be careful about what the messages actually say.

Agreed.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8/25/15 10:50 AM, Jim Nasby wrote:

figuring out the cause of his problem. Given the way the namespace data
structure works, I am not sure that we can realistically detect at line 8
that there was an instance of lab1 earlier, but perhaps we could word the

Are there any other reasons we'd want to improve the ns stuff? Doesn't
seem worth it for just this case, but if there were other nitpicks
elsewhere maybe it is.

Thinking about this some more...

If we added a "prev_label_in_context" field to nsitem and changed how
push worked we could walk the entire chain. Most everything just cares
about the previous level, so I don't think it would be terribly invasive.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Jim Nasby <Jim.Nasby@BlueTreble.com> writes:

On 8/22/15 2:53 PM, Tom Lane wrote:

... Given the way the namespace data
structure works, I am not sure that we can realistically detect at line 8
that there was an instance of lab1 earlier, but perhaps we could word the

Are there any other reasons we'd want to improve the ns stuff? Doesn't
seem worth it for just this case, but if there were other nitpicks
elsewhere maybe it is.

I'm not aware offhand of any other cases where it's not getting the job
done.

error message to cover either possibility. Maybe something like "there is
no label "foo" surrounding this statement"?

"surrounding" seems pretty nebulous. Maybe "no label "foo" in this
context"? I'd say we use the term block, but we differentiate between
blocks and loops. Perhaps it would be best to document namespaces and
make it clear that blocks and loops both use them. :/

I agree that "surrounding" might not be the best word, but it seems more
concrete than "in this context". The point is that the label needs to be
attached to a block/loop that contains the CONTINUE/EXIT statement.
I considered phrasing it as "no label that contains this statement", but
thinking of the label itself as containing anything seemed pretty bogus.

Regardless of that, a hint is probably warranted. "Is "foo" a label for
an adjacent block or loop?"?

Meh. Doesn't do anything for me. If we had positive detection, we could
add an errdetail saying "There is a label "foo", but it's not attached to
a block that encloses this statement.". But without being able to say
that for sure, I think the hint would probably just be confusing.

Hmm ... what do you think of wording the error as "there is no label "foo"
attached to any block enclosing this statement"? That still leaves the
terminology "block" undefined, but it seems better than "any statement
enclosing this statement".

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:

Hmm ... what do you think of wording the error as "there is no label "foo"
attached to any block enclosing this statement"? That still leaves the
terminology "block" undefined, but it seems better than "any statement
enclosing this statement".

Actually, looking at the plpgsql documentation, I see that it is
completely consistent about using the word "block" to refer to
[DECLARE]/BEGIN/END. So we probably can't get away with using the term in
a vaguer sense here. So the wording would have to be "there is no label
"foo" attached to any block or loop enclosing this statement". That's a
tad verbose, but at least it's clear ...

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:

So the wording would have to be "there is no label "foo" attached to
any block or loop enclosing this statement". That's a tad verbose,
but at least it's clear ...

This seems good to me, verbosity notwithstanding.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Tom Lane wrote:

So the wording would have to be "there is no label "foo" attached to
any block or loop enclosing this statement". That's a tad verbose,
but at least it's clear ...

This seems good to me, verbosity notwithstanding.

Hearing no objections, I'll go make it so.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers