Обсуждение: Error message with plpgsql CONTINUE

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.

[1]
decibel@decina.attlocal/50703=# do $$
begin    <<outer>>    for a in 1..3 loop    <<sub>>    BEGIN        <<inner>>        for b in 8..9 loop            if
a=2then                continue sub;            end if;            raise notice '% %', a, b;        end loop inner;
ENDsub;    end loop outer;
 
end;
$$;
NOTICE:  1 8
NOTICE:  1 9
ERROR:  CONTINUE cannot be used outside a loop
CONTEXT:  PL/pgSQL function inline_code_block
decibel@decina.attlocal/50703=#

[2] 
https://github.com/postgres/postgres/blob/83604cc42353b6c0de2a3f3ac31f94759a9326ae/src/pl/plpgsql/src/pl_exec.c#L438
-- 
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:
> 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

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

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

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

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

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

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

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

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

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

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

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

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