Обсуждение: options for RAISE statement

Hello

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

Proposal: http://archives.postgresql.org/pgsql-hackers/2008-04/msg00919.php

I changed keyword from WITH to USING, because I don't would to create
new keyword

RAISE level 'format' [, expression [, ...]] [ USING ( option =
expression [, ... ] ) ];

RAISE EXCEPTION 'Nonexistent ID --> %', user_id
  USING (hint = 'Please, check your user id');

Regards
Pavel Stehule

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>:
> "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 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.
>
> 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
>

"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

>
>                        regards, tom lane
>