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
>
