Re: [HACKERS] errcontext support in PL/Perl
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
Peter Eisentraut pete...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
On tis, 2009-09-15 at 11:32 -0400, Tom Lane wrote: Peter Eisentraut pete...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
Peter Eisentraut pete...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
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 Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
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 Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
Alexey Klyukin al...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] errcontext support in PL/Perl
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) plperl_error_callback_v2.diff Description: Binary data -- Alexey Klyukin http://www.CommandPrompt.com The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
