Обсуждение: 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 >