options for RAISE statement

Pavel Stehule escribi�:

Hello

this patch adds possibility to set additional options (SQLSTATE,
DETAIL, DETAIL_LOG and HINT) for RAISE statement,

Added to May commitfest page, thanks.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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

this patch adds possibility to set additional options (SQLSTATE,
DETAIL, DETAIL_LOG and HINT) for RAISE statement,

I looked this over briefly. A couple of comments:

* Raising errors via hard-coded SQLSTATEs seems pretty unfriendly,
at least for cases where we are reporting built-in errors. Wouldn't
it be better to be able to raise errors using the same SQLSTATE names
that are recognized in EXCEPTION clauses?

* If we are going to let people throw random SQLSTATEs, there had better
be a way to name those same SQLSTATEs in EXCEPTION.

* I don't really like exposing DETAIL_LOG in this. That was a spur of
the moment addition and we might take it out again; I think it's way
premature to set it in stone by exposing it as a plpgsql feature.

* Please avoid using errstart() directly. This is unwarranted intimacy
with elog.h's implementation and I also think it will have unpleasant
behavior if an error occurs while evaluating the RAISE arguments.
(In fact, I think a user could easily force a backend PANIC that way.)
The approved way to deal with ereport options that might not be there
is like this:

ereport(ERROR,
( ...,
have_sqlstate ? errcode(...) : 0,
...

That is, you should evaluate all the options into local variables
and then do one normal ereport call.

* // comments are against our coding conventions.

regards, tom lane

Hello

I thing, all your comments are not problem. I'll send new version this week.

Thank You
Pavel Stehule

2008/5/5 Tom Lane <tgl@sss.pgh.pa.us>:

Hello

I am sending enhanced version of original patch.

2008/5/5 Tom Lane <tgl@sss.pgh.pa.us>:

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

this patch adds possibility to set additional options (SQLSTATE,
DETAIL, DETAIL_LOG and HINT) for RAISE statement,

I looked this over briefly. A couple of comments:

* Raising errors via hard-coded SQLSTATEs seems pretty unfriendly,
at least for cases where we are reporting built-in errors. Wouldn't
it be better to be able to raise errors using the same SQLSTATE names
that are recognized in EXCEPTION clauses?

There are new attribut CONDITION - all defined condition are possible
without duplicit names and category conditions.

example:
RAISE NOTICE 'custom unique violation' USING (CONDITION = 'unique_violation');

* If we are going to let people throw random SQLSTATEs, there had better
be a way to name those same SQLSTATEs in EXCEPTION.

we can trap EXCEPTION defined via SQLSTATE now:

EXCEPTION
WHEN SQLSTATE 22001 THEN ...

* I don't really like exposing DETAIL_LOG in this. That was a spur of
the moment addition and we might take it out again; I think it's way
premature to set it in stone by exposing it as a plpgsql feature.

removed

* Please avoid using errstart() directly. This is unwarranted intimacy
with elog.h's implementation and I also think it will have unpleasant
behavior if an error occurs while evaluating the RAISE arguments.
(In fact, I think a user could easily force a backend PANIC that way.)
The approved way to deal with ereport options that might not be there
is like this:

ereport(ERROR,
( ...,
have_sqlstate ? errcode(...) : 0,
...

That is, you should evaluate all the options into local variables
and then do one normal ereport call.

changed

* // comments are against our coding conventions.

regards, tom lane

removed

Regards
Pavel Stehule

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

I am sending enhanced version of original patch.

Hmm ... this patch seems to have been generated against something
significantly different from HEAD ... was that intentional?

patching file plpgsql.sgml
Hunk #1 succeeded at 2102 (offset -82 lines).
Hunk #3 succeeded at 2167 (offset -82 lines).
Hunk #5 succeeded at 2807 (offset -82 lines).
patching file gram.y
Hunk #1 succeeded at 52 (offset -1 lines).
Hunk #2 succeeded at 141 with fuzz 2 (offset -2 lines).
Hunk #3 succeeded at 1262 (offset -45 lines).
Hunk #4 succeeded at 1314 (offset -2 lines).
Hunk #5 succeeded at 1279 (offset -45 lines).
Hunk #6 succeeded at 1646 (offset -2 lines).
Hunk #7 succeeded at 2703 (offset -144 lines).
patching file pl_comp.c
Hunk #1 succeeded at 1750 (offset -1 lines).
patching file pl_exec.c
Hunk #1 succeeded at 2270 (offset -97 lines).
patching file pl_funcs.c
Hunk #1 succeeded at 1012 (offset -43 lines).
patching file plpgsql.h
Hunk #1 succeeded at 120 (offset -1 lines).
Hunk #2 succeeded at 554 (offset -18 lines).
Hunk #3 succeeded at 808 (offset -1 lines).
patching file plpgsql.out
Hunk #1 FAILED at 3385.
1 out of 1 hunk FAILED -- saving rejects to file plpgsql.out.rej
patching file plpgsql.sql
Hunk #1 FAILED at 2735.
1 out of 1 hunk FAILED -- saving rejects to file plpgsql.sql.rej

regards, tom lane

I am sent two less dependend patch (both modify same files): COPY and
RAISE USING. I am sorry, but I can't to know what commiters will be
apply first. Problem is mainly in regress files because I append
regress test on end of files. But boths are really generated from
current HEAD.

Regards
Pavel Stehule

2008/5/12 Tom Lane <tgl@sss.pgh.pa.us>:

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

I am sending enhanced version of original patch.

Applied with syntax revisions as per pghackers discussion.

I made a couple of other changes too: I took out the restriction against
throwing codes that are category codes, and instead just documented that
that might be a bad idea. I don't see a reason to prohibit that if
people really want to do it. Also, for the few condition names that are
duplicated in plerrcodes.h, I just made it throw the first sqlstate it
finds, rather than complaining. This will work, in the sense that you
can trap the error using the same condition name, and I don't think it's
really important to make the user think about this.

regards, tom lane

2008/5/14 Tom Lane <tgl@sss.pgh.pa.us>:

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

I am sending enhanced version of original patch.

Applied with syntax revisions as per pghackers discussion.

thank you

I made a couple of other changes too: I took out the restriction against
throwing codes that are category codes, and instead just documented that
that might be a bad idea. I don't see a reason to prohibit that if
people really want to do it.

I didn't see a reason to allow it - but I don't see it important

Also, for the few condition names that are

duplicated in plerrcodes.h, I just made it throw the first sqlstate it
finds, rather than complaining. This will work, in the sense that you
can trap the error using the same condition name, and I don't think it's
really important to make the user think about this.

I am afraid so this can be surprise for some people - but again it's
not necessary solve it now.

Regards
Pavel Stehule