Re: 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 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(at)commandprompt(dot)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)

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(at)gmx(dot)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(at)gmx(dot)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(at)gmx(dot)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