Thread: errcontext support in PL/Perl
Hi, While trying to build a custom error reporting function for one of our clients we came across the fact that PL/Perl doesn't set errcontext that we relied on to get the traceback of running functions. Exactly the same problem with PL/Python was fixed recently by Peter Eisentraut (http://archives.postgresql.org/pgsql-committers/2009-07/msg00168.php). Attached is a patch (HEAD) that sets errcontext with PL/Perl function name, making a distinction between compilation and execution stages, fixes error messages where function name was already included in the message itself and updates regression tests. I'll appreciate any suggestions on how to improve it. -- Alexey Klyukin http://www.CommandPrompt.com The PostgreSQL Company - Command Prompt, Inc.
Attachment
Alexey Klyukin wrote: > Attached is a patch (HEAD) that sets errcontext with PL/Perl function > name, making a distinction between compilation and execution stages, > fixes error messages where function name was already included in the > message itself and updates regression tests. I'll appreciate any > suggestions on how to improve it. Hmm, in plperl_exec_callback(), does the global variable work if you call one plperl function from another? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Jul 21, 2009, at 6:39 PM, Alvaro Herrera wrote: > Alexey Klyukin wrote: > >> Attached is a patch (HEAD) that sets errcontext with PL/Perl function >> name, making a distinction between compilation and execution stages, >> fixes error messages where function name was already included in the >> message itself and updates regression tests. I'll appreciate any >> suggestions on how to improve it. > > Hmm, in plperl_exec_callback(), does the global variable work if you > call one plperl function from another? PL/Perl functions can't call each other directly. I don't see any problems with SPI calls: test=# create function perl_log1() returns void language plperl as $$ test$# elog(NOTICE, "Test from function one"); test$# $$ test-# ; CREATE FUNCTION test=# create function perl_log2() returns void language plperl as $ $ elog (NOTICE, "Test from function two"); my $rv = spi_exec_query('SELECT * FROM perl_log1()'); $$; CREATE FUNCTION test=# select perl_log2(); NOTICE: Test from function two CONTEXT: PL/Perl function "perl_log2" NOTICE: Test from function one CONTEXT: PL/Perl function "perl_log1" SQL statement "SELECT * FROM perl_log1()" PL/Perl function "perl_log1" perl_log2 ----------- (1 row) -- Alexey Klyukin http://www.CommandPrompt.com The PostgreSQL Company - Command Prompt, Inc.
Alexey Klyukin wrote: > NOTICE: Test from function one > CONTEXT: PL/Perl function "perl_log1" > SQL statement "SELECT * FROM perl_log1()" > PL/Perl function "perl_log1" Shouldn't the second "PL/Perl function" line say perl_log2 instead? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alexey Klyukin <alexk@commandprompt.com> writes: > On Jul 21, 2009, at 6:39 PM, Alvaro Herrera wrote: >> Hmm, in plperl_exec_callback(), does the global variable work if you >> call one plperl function from another? > PL/Perl functions can't call each other directly. Still, it's poor style to rely on the global variable when you don't have to. Use the error_context.arg field to pass it. regards, tom lane
On Jul 21, 2009, at 7:20 PM, Alvaro Herrera wrote: > Alexey Klyukin wrote: > >> NOTICE: Test from function one >> CONTEXT: PL/Perl function "perl_log1" >> SQL statement "SELECT * FROM perl_log1()" >> PL/Perl function "perl_log1" > > Shouldn't the second "PL/Perl function" line say perl_log2 instead? Hm, yeah, seems to be a problem. I'll change the callback to avoid using global data. -- Alexey Klyukin http://www.CommandPrompt.com The PostgreSQL Company - Command Prompt, Inc.
On Jul 21, 2009, at 7:47 PM, Alexey Klyukin wrote: > > On Jul 21, 2009, at 7:20 PM, Alvaro Herrera wrote: > >> Alexey Klyukin wrote: >> >>> NOTICE: Test from function one >>> CONTEXT: PL/Perl function "perl_log1" >>> SQL statement "SELECT * FROM perl_log1()" >>> PL/Perl function "perl_log1" >> >> Shouldn't the second "PL/Perl function" line say perl_log2 instead? > > Hm, yeah, seems to be a problem. I'll change the callback to avoid > using global data. Attached is the updated version of the patch (the original description is here: http://archives.postgresql.org/pgsql-hackers/2009-07/msg01332.php) that doesn't use global variables. It also shows the last line of the context in the example above correctly: psql (8.5devel) Type "help" for help. test=# select perl_log2(); NOTICE: Test from function two CONTEXT: PL/Perl function "perl_log2" NOTICE: Test from function one CONTEXT: PL/Perl function "perl_log1" SQL statement "SELECT * FROM perl_log1()" PL/Perl function "perl_log2" perl_log2 ----------- (1 row) -- Alexey Klyukin http://www.CommandPrompt.com The PostgreSQL Company - Command Prompt, Inc.
Attachment
On Tue, 2009-07-21 at 20:54 +0300, Alexey Klyukin wrote: > Attached is the updated version of the patch (the original description > is here: http://archives.postgresql.org/pgsql-hackers/2009-07/msg01332.php) > that doesn't use global variables. Patch looks OK. But for extra credit, couldn't we code it so that the context is set before the PL handler is called, so that we get this functionality into all PLs at once?
Peter Eisentraut <peter_e@gmx.net> writes: > But for extra credit, couldn't we code it so that the context is set > before the PL handler is called, so that we get this functionality into > all PLs at once? FWIW, I don't particularly agree with that --- there is no reason to suppose that all PLs will want to do this exactly the same way. Even if they did, the amount of code shared would only be a few lines. It would likely end up being *more* code not less by the time you'd found a way to do it in common (since e.g. the function caches would not be the same for all PLs, if they even had one). regards, tom lane
On tis, 2009-09-15 at 11:32 -0400, Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.net> writes: > > But for extra credit, couldn't we code it so that the context is set > > before the PL handler is called, so that we get this functionality into > > all PLs at once? > > FWIW, I don't particularly agree with that --- there is no reason to > suppose that all PLs will want to do this exactly the same way. I'd imagine that we simply set the context to "$language function $name", probably in fmgr_info_other_lang(). If a language wants more than that, it can set another level of context. Of course this way we would not save much code in, say, PL/Perl, but all the other PLs that are currently not using this stuff at all would get it for free.
Peter Eisentraut <peter_e@gmx.net> writes: > On tis, 2009-09-15 at 11:32 -0400, Tom Lane wrote: >> FWIW, I don't particularly agree with that --- there is no reason to >> suppose that all PLs will want to do this exactly the same way. > I'd imagine that we simply set the context to "$language function > $name", probably in fmgr_info_other_lang(). If a language wants more > than that, it can set another level of context. So if we want to emit something like "$language function $name line $lineno", that now has to be two separate lines of context? There'd be no clean way for the PL to suppress the one it doesn't really need. > Of course this way we > would not save much code in, say, PL/Perl, but all the other PLs that > are currently not using this stuff at all would get it for free. It would be very far from being "for free", because there's no inexpensive way for a general-purpose hook to get hold of either $language or $name without knowing anything about the internals of the PL. It would have to fetch the relevant pg_language and pg_proc rows, the former being unnecessary for the PL itself, and the latter being almost certainly redundant with what the PL is doing. You could eliminate the performance objection perhaps by fetching the rows only if the context hook is actually called, but then you have added failure modes that weren't there before (if the fetch fails for some reason). Also, there is noplace to establish the hook anyway (without adding another layer of call overhead). fmgr_info cannot do it, because it's not in the actual runtime call chain. Between the expense, the low return, and the high probability of not being exactly what's wanted, I don't think this seems like a good design choice. regards, tom lane
From the patch review group this evening, courtesy of Webb Sprague and Brent Dombrowski. I'm replying on their behalf so that this response is in the correct thread. Looks like Peter also reviewed, but I'm forwarding this on anyway. (Note: Problem running regressions because make check doesn't descend automatically into the plperl -- we built with --with-perl successfully, but it doesn't change the behavior of make check. However, the tests do work with patch.) Per the wiki guidelines: == Submission review == * Is the patch in the standard form? Yes, * Does it apply cleanly to the current CVS HEAD? Yes * Does it include reasonable tests, docs, etc? Inline comments suffice. == Usability review == Read what the patch is supposed to do, and consider: * Does the patch actually implement that? Yes, built in to the expected test output. * Do we want that? Yes, based on the patch to Python that does similar stuff. * Do we already have it? No * Does it follow SQL spec, or the community-agreed behavior? There was extensive discussion on the list about the desirability of the context messages, and the community seems to agree in the utility * Are there dangers? No. (??) * Have all the bases been covered? We guess... == Feature test == Apply the patch, compile it and test: * Does the feature work as advertised? Make check passes, and expected output is provided (problem with automatically checking all) * Are there corner cases the author has failed to consider? Can't think of any -- popping off an empty stack or such worries me, but I couldn't determine if there was a risk of this in the code. == Performance review == * Does the patch slow down simple tests? No. * If it claims to improve performance, does it? NA * Does it slow down other things? No (?) == Coding review == Read the changes to the code in detail and consider: * Does it follow the project [http://developer.postgresql.org/pgdocs/postgres/source.html coding guidelines]? Yes * Are there portability issues? None that we can see * Will it work on Windows/BSD etc? No issues we can see * Are the comments sufficient and accurate? Yes * Does it do what it says, correctly? Yes * Does it produce compiler warnings? No * Can you make it crash? No == Architecture review == Consider the changes to the code in the context of the project as a whole: * Is everything done in a way that fits together coherently with other features/modules? Yes * Are there interdependencies than can cause problems? No, it is a simple set of changes all within plperl
On Tue, 2009-07-21 at 20:54 +0300, Alexey Klyukin wrote: > Attached is the updated version of the patch (the original description > is here: http://archives.postgresql.org/pgsql-hackers/2009-07/msg01332.php) > that doesn't use global variables. It also shows the last line of > the context in the example above correctly: committed
Selena Deckelmann escribió: > From the patch review group this evening, courtesy of Webb Sprague and > Brent Dombrowski. I'm replying on their behalf so that this response > is in the correct thread. > > Looks like Peter also reviewed, but I'm forwarding this on anyway. > > (Note: Problem running regressions because make check doesn't descend > automatically into the plperl -- we built with --with-perl > successfully, but it doesn't change the behavior of make check. > However, the tests do work with patch.) You have to use "make -C src/pl installcheck". -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support