SEO

"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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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
>

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

"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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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
>

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches