plpgsql raise - parameters can be expressions

pokus=# create or replace function x() returns void as $$
declare c integer[] = '{10,20,30}'; a integer = 3;b record;
begin b := row(1,2);
raise notice 'sss % % % % % % % % %', interval '23 hour',
1, current_user, c,now(), c[1],
(select * from fx where 1 = 0 limit 1),
null,current_timestamp::timestamp(0);
end; $$ language plpgsql;
CREATE FUNCTION
pokus=# select x();
NOTICE: sss 23:00:00 1 root {10,20,30} 2005-06-13 07:06:07.43569+02 10
<NULL> <NULL> 2005-06-13 07:
06:07

Regards
Pavel Stehule

I like :)

+1

On Jun 13, 2005, at 2:07 PM, Pavel Stehule wrote:

I did trivial patch which eliminate limit raise command. Using
expressions instead of variables are a little bit expensive, but
little.

I'm right in thinking this essentially does the same thing as first
assigning the value of the expression to a variable and then using
the variable in the RAISE statement, correct? This patch just
eliminates the step of assigning the value of the expression to the
variable? If so, I'd expect the cost to be in line with the cost of
explicitly assigning to a variable.

Definitely nifty!

Michael Glaesemann
grzm myrealbox com

On Mon, 13 Jun 2005, Michael Glaesemann wrote:

On Jun 13, 2005, at 2:07 PM, Pavel Stehule wrote:

I did trivial patch which eliminate limit raise command. Using
expressions instead of variables are a little bit expensive, but
little.

I'm right in thinking this essentially does the same thing as first
assigning the value of the expression to a variable and then using
the variable in the RAISE statement, correct? This patch just
eliminates the step of assigning the value of the expression to the
variable? If so, I'd expect the cost to be in line with the cost of
explicitly assigning to a variable.

true, total cost is less

Pavel

Pavel Stehule wrote:

I did trivial patch which eliminate limit raise command.

Looks pretty good. Attached is a cleaned-up version that I'll apply to
HEAD tomorrow, barring any objections.

BTW, one regression is that an undefined variable in the RAISE list is
no longer a compile-time error:

create function foo() returns void as '
begin
raise notice ''hello, world: %'', baz;
end;' language plpgsql;

neilc=# select foo();
ERROR: column "baz" does not exist

I don't see an easy way to get around this, though, and it's not too
concerning. Amusingly it does completely break the SQLSTATE and SQLERRM
tests we added a few days ago :)

BTW, another easy improvement in this area is changing the RAISE format
string to allow it to be an expression, rather than only a string literal.

-Neil

I don't see an easy way to get around this, though, and it's not too
concerning. Amusingly it does completely break the SQLSTATE and SQLERRM
tests we added a few days ago :)

Yes, it's true. But with simple plpgsql parser isn't possible expect
miracles :). I think so there is some space for modifications -
read_sql_construct - add control for availability sql params. You can
solve all errors where is expr_until_semi.

This is exactly same situation like

create function ...
begin
execute 'select '||a;
end; $$

it generate runtime error

BTW, another easy improvement in this area is changing the RAISE format
string to allow it to be an expression, rather than only a string literal.

No problem. But it's maybe big change. And I don't see using format string
too much limiting.

Pavel

Pavel Stehule wrote:

I did trivial patch which eliminate limit raise command.

In thinking about this some more, it would be nice to be able to use
exec_eval_expr() to reduce expression evaluation overhead for simple
RAISE parameters. It is easy enough to refactor the current
exec_eval_expr() code so this is possible, but it will be of only
limited use: PL/PgSQL currently only considers expressions that result
in a single attribute as sufficiently simple that they can be evaluated
via ExecEvalExpr(). I wonder how much work it would take to lift this
restriction...

-Neil

Neil Conway <neilc@samurai.com> writes:

BTW, one regression is that an undefined variable in the RAISE list is
no longer a compile-time error:
...
I don't see an easy way to get around this, though, and it's not too
concerning. Amusingly it does completely break the SQLSTATE and SQLERRM
tests we added a few days ago :)

That doesn't bother me either, seeing that an undefined variable isn't
detected at compile time anywhere else. However, fixing the SQLSTATE
tests by removing them doesn't seem like a great solution ...

BTW, another easy improvement in this area is changing the RAISE format
string to allow it to be an expression, rather than only a string literal.

I would sort of have expected this to get done at the same time.

Actually, the reason I didn't do something about RAISE in 8.0 was that
I thought we should reconsider the whole design of the statement: it
desperately needs to be fixed so that you can specify the SQLSTATE to
be thrown, and so that you can re-throw the same exception you caught.
(Note that SQLERRM is not really a solution to that: you might think
something like "RAISE EXCEPTION SQLSTATE, '%', SQLERRM" would do,
but it loses information, namely all the auxiliary fields.)

Ideas?

regards, tom lane

That doesn't bother me either, seeing that an undefined variable isn't
detected at compile time anywhere else. However, fixing the SQLSTATE
tests by removing them doesn't seem like a great solution ...

BTW, another easy improvement in this area is changing the RAISE format
string to allow it to be an expression, rather than only a string literal.

I would sort of have expected this to get done at the same time.

Actually, the reason I didn't do something about RAISE in 8.0 was that
I thought we should reconsider the whole design of the statement: it
desperately needs to be fixed so that you can specify the SQLSTATE to
be thrown, and so that you can re-throw the same exception you caught.
(Note that SQLERRM is not really a solution to that: you might think
something like "RAISE EXCEPTION SQLSTATE, '%', SQLERRM" would do,
but it loses information, namely all the auxiliary fields.)

Ideas?

only RAISE? Without parameters can be used only in block. It's same scope
like SQLERRM and SQLSTATE.

Oracle can define variables with type EXCEPTION. This all what we need -
identificator. For value of exception Oracle use PRAGMA EXCEPTION_INIT -
for SQLCODE value. PostgreSQL don't suport SQLCODE, then PRAGMA is
irelevant, but what:

DECLARE my_exception EXCEPTION = '22012'; -- division by zero
BEGIN
RAISE my_exception; -- named exception; -- no params
EXCEPTION WHEN division_by_zero THEN
my_exception ~ division by zero
END;

----------or----------------
EXCEPTIO WHEN my_exception THEN
...
END

all variants are legal. I can use user's exception
with default unique value from predefined interval too.

DECLARE my_exception EXCEPTION;

regards
Pavel Stehule

p.s.

Pavel Stehule <stehule@kix.fsv.cvut.cz> writes:

Ideas?

only RAISE? Without parameters can be used only in block. It's same scope
like SQLERRM and SQLSTATE.

OK, but what about the other problem --- throwing a non-default SQLSTATE
along with your custom error message?

RAISE my_exception; -- named exception; -- no params

Definitely not adequate. Maybe we could do

RAISE my_exception, 'format', params;

I think it would be better to do something like

RAISE ERROR my_exception, 'format', params;

since this won't be ambiguous with the existing variants of RAISE;
without the ERROR keyword it'd be necessary to forbid exception names
from being DEBUG, LOG, etc.

I like your DECLARE syntax better than Oracle's incredibly ugly "pragma"
notation, and we can't expect to be exactly compatible with the pragma
anyway since the pragma is associating the name with an integer code,
which is not what we are going to use. So that part seems good.

I assume you intend to allow "RAISE ERROR division_by_zero ..." and
"EXCEPTION WHEN my_exception THEN ...", right? That is, declared
exception names are interchangeable with the predefined ones for
both throwing and catching errors.

I can use user's exception with default unique value from predefined
interval too.

I see zero value in that. The only reason for declaring a specific
user-defined exception code is so you can identify it from elsewhere,
like in your application --- so not knowing which code it's getting
makes it useless.

regards, tom lane

I would sort of have expected this to get done at the same time.

Actually, the reason I didn't do something about RAISE in 8.0 was that
I thought we should reconsider the whole design of the statement: it
desperately needs to be fixed so that you can specify the SQLSTATE to
be thrown, and so that you can re-throw the same exception you caught.
(Note that SQLERRM is not really a solution to that: you might think
something like "RAISE EXCEPTION SQLSTATE, '%', SQLERRM" would do,
but it loses information, namely all the auxiliary fields.)

There is space for future version. I think for todo for plpgsql

o rethrow exception in exception handler block
o user defined exceptions
o mapping user's exception on system's exception

My previous mail isn't correct. I need somewhere specify errmsg text, and
ofcourse params.

???

declare myexcept exception = 'xxxxx';
begin;
assoc(myexcept, 'my message % % %');
raise myexcept, param1, param2, ...

default level of user exception is exception.

regards
Pavel Stehule

Pavel Stehule <stehule@kix.fsv.cvut.cz> writes:

declare myexcept exception = 'xxxxx';
begin;
assoc(myexcept, 'my message % % %');
raise myexcept, param1, param2, ...

Our experience in writing the backend is that hard-wiring a single error
message text for a SQLSTATE code is the wrong thing. The SQLSTATE codes
are relatively coarse-grained (which is usually the right thing from the
perspective of an application trying to match a SQLSTATE) and there is
very often room for the text message to give more detail than the
SQLSTATE alone conveys.

Also, the above seems highly error prone to me since it decouples the
format string from the parameters. This is even worse for built-in
exception codes since the writer of a plpgsql function shouldn't assume
that he knows exactly what % parameters a built-in exception's format
string would use.

So for all these reasons, I think that only the SQLSTATE should be
associated with the exception name. The format string should be part
of the RAISE command.

regards, tom lane

On Mon, 13 Jun 2005, Tom Lane wrote:

Pavel Stehule <stehule@kix.fsv.cvut.cz> writes:

Ideas?

only RAISE? Without parameters can be used only in block. It's same scope
like SQLERRM and SQLSTATE.

OK, but what about the other problem --- throwing a non-default SQLSTATE
along with your custom error message?

I understand. Its not problem when I have SQLERRM non paramized ~ constant
string - I can get custom error message without changes. Problem is when
error message is parametrized. What is better, rebuild this string or use
old. Maybe its reason why isn't possible easy change errstr in Oracle.

Next problem, visibility custom exceptions. When I define exception
variable I can't rethrown exceptions outside block when is defined. What
is outside - some custom exception?

I think it would be better to do something like

RAISE ERROR my_exception, 'format', params;

disadvantage - I have to define format string everywhere where I wont to
raise exception. Idea is similar MsSQL. Then is necessary define only
interval for user's exception. But it's not too much readable.

I like your DECLARE syntax better than Oracle's incredibly ugly "pragma"
notation, and we can't expect to be exactly compatible with the pragma
anyway since the pragma is associating the name with an integer code,
which is not what we are going to use. So that part seems good.

From OOP view exception is object. But I need define more properties than
one. SQLSTATE is only first, second message, level, meybe next

DECLARE myexception EXCEPTION VALUE 'xxxx' MESSAGE 'xxx % % %';
or
DECLARE myexception EXCEPTION MESSAGE 'xxxx % %' = '3333'; -- optional
DECLARE myexception EXCEPTION = '2222'; -- short no message

I think so user's exception must not have errmsg. When I define own
exception I expect own exception handler. This is only more readable
style. I can have one global variable and one exception. It's can be short
way to exception handler

I think so we need more then one exception level. I can use
user's exception for easy way of write to log. Every user's exception can
by handled - this is differnt then RAISE NOTICE, any user's exception
have to be handled - equal RAISE EXCEPTION (default have to be handled),
any user's excp. can be logged. This is big theory :-), pardon

DECLARE myex EXCEPTION TOLOG NOERROR CATCHED MESSAGE 'aaaa' VALUE '10';

short myex EXCEPTION = '10';
~ ERROR CATCHED MESSAGE ''

I assume you intend to allow "RAISE ERROR division_by_zero ..." and
"EXCEPTION WHEN my_exception THEN ...", right? That is, declared
exception names are interchangeable with the predefined ones for
both throwing and catching errors.

Yes, I wont it. When I use SQLSTATE from system's SQLSTATEs, I speek
my exception can by handled by any general handler. But I have possibility
logging, level, ...

this is way for throwing system's exception. I think will be better
prohibit throwing system's exceptions directly. But I can associate my
exception with system's exception, and throw my exception.

I can use user's exception with default unique value from predefined
interval too.

I see zero value in that. The only reason for declaring a specific
user-defined exception code is so you can identify it from elsewhere,
like in your application --- so not knowing which code it's getting
makes it useless.

There are two questions?

Have Every user's exception unique SQLSTATE value?
Will be exist any interval for user's exception SQLSTATES?

My answer is true for all. If it's true, then I can check one SQLSTATE
exception handler in block. In my example is clean nonsens

DECLARE e EXCEPTION = '22....'; -- div_by_zero
BEGIN
RAISE ERROR e;
EXCEPTION WHEN e THEN ..
EXCEPTION WHEN div_by_zero THEN ..

NONSENS e and div_by_zero are synonyms

Regards
Pavel Stehule

Our experience in writing the backend is that hard-wiring a single error
message text for a SQLSTATE code is the wrong thing. The SQLSTATE codes
are relatively coarse-grained (which is usually the right thing from the
perspective of an application trying to match a SQLSTATE) and there is
very often room for the text message to give more detail than the
SQLSTATE alone conveys.

I can define more user's exceptions. And every can have own message text.
It's personal preference. I prefere one exception, one parametrized
message text. It's not important on procedure level. But If can be
possible define exceptions for schema, then I can share message texts.

Also, the above seems highly error prone to me since it decouples the
format string from the parameters. This is even worse for built-in
exception codes since the writer of a plpgsql function shouldn't assume
that he knows exactly what % parameters a built-in exception's format
string would use.

Yes, it's can be source of errors. But I can check it in compile time (not
now, or after apply patch (if will be).

So for all these reasons, I think that only the SQLSTATE should be
associated with the exception name. The format string should be part
of the RAISE command.

my ideas are only proposal. Nothing more. What I think is important

o using expr in raise params
o set SQLSTATE for better diagnostic
o raising system's exceptions

all next is unnecessary luxus.

But the idea EXCEPTION's variable is good, and can be easy evolved.

Regards
Pavel Stehule

Pavel Stehule <stehule@kix.fsv.cvut.cz> writes:

Next problem, visibility custom exceptions. When I define exception
variable I can't rethrown exceptions outside block when is defined. What
is outside - some custom exception?

I don't think this is an issue. A custom exception is really just a
name for a SQLSTATE value, so you can throw it in any case. An outer
block that does not know that name can only catch it as WHEN OTHERS,
but so what?

I would also expect that matching is by SQLSTATE, that is if I write

DECLARE foo EXCEPTION = '12345';
...
RAISE ERROR foo, ...;

then some outer block written as

DECLARE bar EXCEPTION = '12345';
...
EXCEPTION WHEN bar THEN ...

would catch this error.

disadvantage - I have to define format string everywhere where I wont to
raise exception.

Why is that a disadvantage? You should not be able to throw an error
without providing a useful message --- that's just basic good
programming.

From OOP view exception is object. But I need define more properties than
one. SQLSTATE is only first, second message, level, meybe next

I think we are better off defining exception names as SQLSTATEs and
nothing else. That's essentially how we have done it in the backend
code and it has worked well over a very large body of code. You are
arguing for a less flexible definition on the basis of nothing more
than a vague appeal to "OOP principles". You have neither stated
exactly which principles nor exactly why they dictate this choice,
so I see nothing convincing in your argument.

I think so we need more then one exception level. I can use
user's exception for easy way of write to log.

Well, can we get away with making the syntax be

RAISE level [ exception_name , ] format_string [ , parameters ] ;

where "level" is one of the already-defined level keywords? Normally
I would think that this would be unacceptably ambiguous, but as long as
the exception_name is constrained to be either a built-in or previously
defined exception name, this is probably workable from a syntactic point
of view.

regards, tom lane

I would also expect that matching is by SQLSTATE, that is if I write

DECLARE foo EXCEPTION = '12345';
...
RAISE ERROR foo, ...;

then some outer block written as

DECLARE bar EXCEPTION = '12345';
...
EXCEPTION WHEN bar THEN ...

if we accept exception is like variable, then there is rules for
variables. I don't see problem there.

EXCEPTION WHEN bar is equivalent of

EXCEPTION SQLSTATE = 12345 THEN

where I see bar, I can see bar. But isn't possible two exception handlers
in one block with one SQLSTATE.

would catch this error.

disadvantage - I have to define format string everywhere where I wont to
raise exception.

Why is that a disadvantage? You should not be able to throw an error
without providing a useful message --- that's just basic good
programming.

it's ok. Exception without error message is wrong. One type of exception
with different error messages (parametrized message is ok) is wrong too.
But it's my personal opinion. Maybe one message string is not sufficient,
better is message string and hint string (like now).

From OOP view exception is object. But I need define more properties than
one. SQLSTATE is only first, second message, level, meybe next

I think we are better off defining exception names as SQLSTATEs and
nothing else. That's essentially how we have done it in the backend
code and it has worked well over a very large body of code. You are
arguing for a less flexible definition on the basis of nothing more
than a vague appeal to "OOP principles". You have neither stated
exactly which principles nor exactly why they dictate this choice,
so I see nothing convincing in your argument.

I have less. All is only my ideas. I don't wont PLPGSQL like full OOP
language. I speak only about possible ways now. I see usefull global
definition of exception on package (or schema, database) level - like
others db objects (sequences). Packages not exists and all is in future.
On procedural level I have to agree with you. Your syntax don't exude
"my" syntax.

If I choise level, format_string,.. in raise stmt, then are used this
params. If not, then are used default parames (in future).

I think so we need more then one exception level. I can use
user's exception for easy way of write to log.

Well, can we get away with making the syntax be

RAISE level [ exception_name , ] format_string [ , parameters ] ;

I agree. I unlike big steps.

About level: I think so already defined levels are good. I have idea about
somethink between levels NOTICE and EXCEPTION. I can't catch NOTICE and I
have to catch EXCEPTION, maybe RAISE EVENT. I can catch it, if I wont. And
this level don't rollback transaction, and should be return back from
exception handler. It's more like calling subrutine. Can be usefull.

Question: What do you think about

I specify minimal level of event in log. But when I have user's exception
I can specify list of user's exception for log.

like

log_level NOTICE
log_exceptions myexception1, myexception2, ...

where "level" is one of the already-defined level keywords? Normally
I would think that this would be unacceptably ambiguous, but as long as
the exception_name is constrained to be either a built-in or previously
defined exception name, this is probably workable from a syntactic point
of view.

Pavel Stehule

Tom Lane wrote:

That doesn't bother me either, seeing that an undefined variable isn't
detected at compile time anywhere else. However, fixing the SQLSTATE
tests by removing them doesn't seem like a great solution ...

Yeah, true, I can just invoke the function to trigger the undefined
variable error.

Actually, the reason I didn't do something about RAISE in 8.0 was that
I thought we should reconsider the whole design of the statement

The ensuing discussion on this sounds good to me; should I apply Pavel's
RAISE patch now, or wait for the subsequent work on specifying a
particular SQLSTATE?

-Neil

Neil Conway <neilc@samurai.com> writes:

Tom Lane wrote:

Actually, the reason I didn't do something about RAISE in 8.0 was that
I thought we should reconsider the whole design of the statement

The ensuing discussion on this sounds good to me; should I apply Pavel's
RAISE patch now, or wait for the subsequent work on specifying a
particular SQLSTATE?

The patch seems to me to be OK as far as it goes. I brought up the
other points only because I wanted to be sure that it wouldn't be
inconsistent with the next step; but it seems we're pretty well agreed
that we aren't going to do anything that would break this. So I have no
problem with applying as-is, rather than waiting for an all-inclusive
patch.

But you had mentioned wanting to look at reducing overhead by using
exec_eval_expr(); were you intending to do that before committing?

As far as the subsequent discussion itself goes, Pavel and I seem to
be pretty unsuccessful at convincing each other of our respective
visions of what an exception ought to be. Any opinions? Should
we be taking this thread to -hackers for a wider audience?

regards, tom lane

Tom Lane wrote:

But you had mentioned wanting to look at reducing overhead by using
exec_eval_expr(); were you intending to do that before committing?

I'm a bit busy with other matters at the moment, so I'll probably look
at it later. One question is whether we should make use of
exec_eval_expr() by representing the RAISE parameter list as a list of
expressions (each of which would likely be simple enough to evaluate via
ExecEvalExpr()), or whether we want to extend exec_eval_expr() to handle
expressions that yield multiple attributes.

-Neil

Neil Conway <neilc@samurai.com> writes:

... One question is whether we should make use of
exec_eval_expr() by representing the RAISE parameter list as a list of
expressions (each of which would likely be simple enough to evaluate via
ExecEvalExpr()), or whether we want to extend exec_eval_expr() to handle
expressions that yield multiple attributes.

I'd lean to the former myself --- which actually does suggest that this
patch is not ready for application yet, because it banks on the
assumption that "x,y,z" should be treated as a single expression.

Now that I think about it, the amount of overhead in that assumption is
pretty high: there's tuple construction and deconstruction involved,
no matter how simple the individual datatypes are. So I'd definitely
prefer to see it changed.

regards, tom lane