Обсуждение: 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
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
[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=2 then
continue sub;
end if;
raise notice '% %', a, b;
end loop inner;
END sub;
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
--
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: > 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
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
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
Вложения
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