plperl and inline functions -- first draft

Joshua Tolley wrote:

I've been trying to make pl/perl support 8.5's inline functions, with the
attached patch.

Wow, this is the second time this week that people have produced patches
for stuff I was about to do. Cool!

The basics seem to be there, with at least one notable
exception, namely that plperl functions can do stuff only plperlu should do. I
presume this is because I really don't understand yet how plperl's trusted
interpreter initialization works, and have simply copied what looked like
important stuff from the original plperl call handler.

I'll check that out.

cheers

andrew

On Thu, Nov 05, 2009 at 05:51:45PM -0500, Andrew Dunstan wrote:

Joshua Tolley wrote:

I've been trying to make pl/perl support 8.5's inline functions, with the
attached patch.

Wow, this is the second time this week that people have produced patches
for stuff I was about to do. Cool!

Well, I warmed up with PL/LOLCODE :)

The basics seem to be there, with at least one notable
exception, namely that plperl functions can do stuff only plperlu should do. I
presume this is because I really don't understand yet how plperl's trusted
interpreter initialization works, and have simply copied what looked like
important stuff from the original plperl call handler.

I'll check that out.

Many thanks.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

Joshua Tolley wrote:

The basics seem to be there, with at least one notable
exception, namely that plperl functions can do stuff only plperlu should do. I
presume this is because I really don't understand yet how plperl's trusted
interpreter initialization works, and have simply copied what looked like
important stuff from the original plperl call handler.

I'll check that out.

Ok, I have a handle on the trusted/nontrusted issue. But I think the
piece that's missing here is that it needs to save the calling context
etc. and use PG_TRY() and friends, just like plperl_call_handler(). I'll
work on that.

cheers

andrew

I wrote:

Ok, I have a handle on the trusted/nontrusted issue. But I think the
piece that's missing here is that it needs to save the calling context
etc. and use PG_TRY() and friends, just like plperl_call_handler().
I'll work on that.

OK, I committed the previously discussed change to store the language
trusted flag in the InlineCodeBlock structure. Following that, here is
my reworking of Josh's patch for DO blocks for plperl.

Missing are docs and regression tests.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

! * plperl_call_handler and plperl_inline_handler are the only
! * externally-visible parts of the plperl call interface. The Postgres function
! * and trigger managers call plperl_call_handler to execute a perl function, and
! * call plperl_inline_handler to execute plperl code in a DO statement.

This comment should be updated to mention the validator. (What it
replaces was wrong before, but that's no excuse for not making it
right while you're touching it.)

The spacing seems a bit random too. pgindent will fix some of that,
but it's not very bright about making vertical spacing (ie extra
blank lines) consistent.

regards, tom lane

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

! * plperl_call_handler and plperl_inline_handler are the only
! * externally-visible parts of the plperl call interface. The Postgres function
! * and trigger managers call plperl_call_handler to execute a perl function, and
! * call plperl_inline_handler to execute plperl code in a DO statement.

This comment should be updated to mention the validator. (What it
replaces was wrong before, but that's no excuse for not making it
right while you're touching it.)

The spacing seems a bit random too. pgindent will fix some of that,
but it's not very bright about making vertical spacing (ie extra
blank lines) consistent.

OK, I'll clean it up.

cheers

andrew

On Fri, Nov 06, 2009 at 06:37:38PM -0500, Andrew Dunstan wrote:

I wrote:

Ok, I have a handle on the trusted/nontrusted issue. But I think the
piece that's missing here is that it needs to save the calling context
etc. and use PG_TRY() and friends, just like plperl_call_handler().
I'll work on that.

OK, I committed the previously discussed change to store the language
trusted flag in the InlineCodeBlock structure. Following that, here is
my reworking of Josh's patch for DO blocks for plperl.

Missing are docs and regression tests.

Attached is a cleaned up comment with documentation. I looked through the
regression tests and didn't find any that used plperl -- should we add one for
this (or for this and all kinds of other stuff)? Is there some way to make
running the regression test conditional on having built --with-perl in the
first place?

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

Joshua Tolley wrote:

I looked through the
regression tests and didn't find any that used plperl -- should we add one for
this (or for this and all kinds of other stuff)? Is there some way to make
running the regression test conditional on having built --with-perl in the
first place?

Look in src/pl/plperl/{sql,expected}

cheers

andrew

On Fri, Nov 06, 2009 at 09:53:20PM -0500, Andrew Dunstan wrote:

Joshua Tolley wrote:

I looked through the
regression tests and didn't find any that used plperl -- should we add one for
this (or for this and all kinds of other stuff)? Is there some way to make
running the regression test conditional on having built --with-perl in the
first place?

Look in src/pl/plperl/{sql,expected}

Ok, updated patch attached. As far as I know, this completes all outstanding
issues:

1) weird comment in plperl.c is corrected and formatted decently
2) plperlu vs. plperl actually works (thanks again, Andrew)
3) docs included
4) regression tests included

Some items of note include that this makes the regression tests add not only
plperl to the test database but also plperlu, which is a new thing. I can't
see why this might cause problems, but thought I'd mention it. The tests
specifically try to verify that plperl doesn't allow 'use Data::Dumper', and
plperlu does. Since Data::Dumper is part of perl core, that seemed safe, but
it is another dependency, and perhaps we don't want to do that. If not, is
there some other useful way of testing plperlu vs. plperl, and does it really
matter?

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

On Fri, Nov 06, 2009 at 09:53:20PM -0500, Andrew Dunstan wrote:

Joshua Tolley wrote:

I looked through the
regression tests and didn't find any that used plperl -- should we add one for
this (or for this and all kinds of other stuff)? Is there some way to make
running the regression test conditional on having built --with-perl in the
first place?

Look in src/pl/plperl/{sql,expected}

cheers

andrew

FWIW, I've added this to the upcoming commitfest page.

https://commitfest.postgresql.org/action/patch_view?id=206

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

Joshua Tolley wrote:

Some items of note include that this makes the regression tests add not only
plperl to the test database but also plperlu, which is a new thing. I can't
see why this might cause problems, but thought I'd mention it. The tests
specifically try to verify that plperl doesn't allow 'use Data::Dumper', and
plperlu does. Since Data::Dumper is part of perl core, that seemed safe, but
it is another dependency, and perhaps we don't want to do that. If not, is
there some other useful way of testing plperlu vs. plperl, and does it really
matter?

Loading both plperl and plperlu could have problems, as there are some
platforms where we can't use them both in the same session, due to some
perl oddities. We would need to test this on one such - I don't recall
which they are.

"Config" might be a better choice than "Data::Dumper". The Perl team or
some packagers could drop Data::Dumper some day, but they aren't likely
to drop Config.

cheers

andrew

2009/11/10 Joshua Tolley <eggyknap@gmail.com>:

Ok, updated patch attached. As far as I know, this completes all outstanding
issues:

Hi Joshua,

I'm taking a look at this patch for the commitfest. I see that Andrew
has already taken an interest in the technical aspects of the patch,
so I'll focus on submission/code style/documentation.

I noticed that there was a fairly large amount of bogus/inconsistent
whitespace in the patch, particularly in the body of
plperl_inline_handler(). Some of the lines were indented with tabs,
others with spaces. You should stick with tabs. There were also a
lot of lines with a whole lot of trailing whitespace at the end.

See attached patch which repairs the whitespace. I see you generated
the patch with git, so I recommend `git diff --check`, it'll helpfully
report about some types of whitespace error.

In the documentation you refer to this feature as "inline functions".
I think this might be mixing up the terminology ... although the code
refers to "inline handlers" internally, the word "inline" doesn't
appear in the user-facing documentation for the DO command. Instead
they are referred to as "anonymous code blocks". I think it would
improve consistency if the PL/Perl mention used the same term.

Apart from those minor quibbles, the patch appears to apply, compile
and test fine, and work as advertised.

Cheers,
BJ

On Sun, Nov 15, 2009 at 12:10:33PM +1100, Brendan Jurd wrote:

I noticed that there was a fairly large amount of bogus/inconsistent
whitespace in the patch, particularly in the body of
plperl_inline_handler(). Some of the lines were indented with tabs,
others with spaces. You should stick with tabs. There were also a
lot of lines with a whole lot of trailing whitespace at the end.

Thanks -- I tend to forget whitespace :)

In the documentation you refer to this feature as "inline functions".
I think this might be mixing up the terminology ... although the code
refers to "inline handlers" internally, the word "inline" doesn't
appear in the user-facing documentation for the DO command. Instead
they are referred to as "anonymous code blocks". I think it would
improve consistency if the PL/Perl mention used the same term.

I can accept that argument. The attached patch modifies the documentation, and
fixes another inconsistency I found.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

2009/11/17 Joshua Tolley <eggyknap@gmail.com>:

On Sun, Nov 15, 2009 at 12:10:33PM +1100, Brendan Jurd wrote:

I noticed that there was a fairly large amount of bogus/inconsistent
whitespace

...

Thanks -- I tend to forget whitespace :)

In the documentation you refer to this feature as "inline functions".
I think this might be mixing up the terminology

...

I can accept that argument. The attached patch modifies the documentation, and
fixes another inconsistency I found.

Cool. I have no gripes with the revised patch. I'm marking this as
ready for committer now. Thanks!

Cheers,
BJ

On Wed, Nov 18, 2009 at 09:35:35AM +1100, Brendan Jurd wrote:

2009/11/17 Joshua Tolley <eggyknap@gmail.com>:

On Sun, Nov 15, 2009 at 12:10:33PM +1100, Brendan Jurd wrote:

I noticed that there was a fairly large amount of bogus/inconsistent
whitespace

...

Thanks -- I tend to forget whitespace :)

In the documentation you refer to this feature as "inline functions".
I think this might be mixing up the terminology

...

I can accept that argument. The attached patch modifies the documentation, and
fixes another inconsistency I found.

Cool. I have no gripes with the revised patch. I'm marking this as
ready for committer now. Thanks!

Thanks to you, as well, and Andrew for his work.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

On Nov 9, 2009, at 6:07 PM, Joshua Tolley wrote:

Ok, updated patch attached. As far as I know, this completes all outstanding
issues:

1) weird comment in plperl.c is corrected and formatted decently
2) plperlu vs. plperl actually works (thanks again, Andrew)
3) docs included
4) regression tests included

Some items of note include that this makes the regression tests add not only
plperl to the test database but also plperlu, which is a new thing. I can't
see why this might cause problems, but thought I'd mention it. The tests
specifically try to verify that plperl doesn't allow 'use Data::Dumper', and
plperlu does. Since Data::Dumper is part of perl core, that seemed safe, but
it is another dependency, and perhaps we don't want to do that. If not, is
there some other useful way of testing plperlu vs. plperl, and does it really
matter?

I've noticed that the patch doesn't install current_call_data before calling plperl_call_perl_func, although it saves and restores its previous value. This breaks spi code, which relies on current_call_data->prodesc, i.e.:

postgres=# DO $$ $result = spi_exec_query("select 1"); $$ LANGUAGE plperl;

server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

rogram received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
0x00000001006f0336 in plperl_spi_exec (query=0x1007ecb60 "select 1", limit=0) at plperl.c:1895
warning: Source file is more recent than executable.
1895 spi_rv = SPI_execute(query, current_call_data->prodesc->fn_readonly,
(gdb) bt
#0 0x00000001006f0336 in plperl_spi_exec (query=0x1007ecb60 "select 1", limit=0) at plperl.c:1895

Also, a call to to plperl_call_perl_func should be cast to void to avoid a possible compiler warning (although It doesn't emit one on my system):

(void) plperl_call_perl_func(&desc, &fake_fcinfo);

--
Alexey Klyukin http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc

Alexey Klyukin wrote:

I've noticed that the patch doesn't install current_call_data before calling plperl_call_perl_func, although it saves and restores its previous value. This breaks spi code, which relies on current_call_data->prodesc, i.e.:

postgres=# DO $$ $result = spi_exec_query("select 1"); $$ LANGUAGE plperl;

Yeah, good catch. We need to lift some stuff out of
plperl_func_handler(), because this code bypasses that. Not only setting
the call_data but also connectin g to the SPI manager and maybe one or
two other things.

Also, a call to to plperl_call_perl_func should be cast to void to avoid a possible compiler warning (although It doesn't emit one on my system):

(void) plperl_call_perl_func(&desc, &fake_fcinfo);

Right.

cheers

andrew

On Tue, Nov 17, 2009 at 06:05:19PM -0500, Andrew Dunstan wrote:

Alexey Klyukin wrote:

I've noticed that the patch doesn't install current_call_data before calling plperl_call_perl_func, although it saves and restores its previous value. This breaks spi code, which relies on current_call_data->prodesc, i.e.:

postgres=# DO $$ $result = spi_exec_query("select 1"); $$ LANGUAGE plperl;

Yeah, good catch. We need to lift some stuff out of
plperl_func_handler(), because this code bypasses that. Not only setting
the call_data but also connectin g to the SPI manager and maybe one or
two other things.

I kept thinking I had to test SPI, but I guess I hadn't ever done it. The
attached takes care of such stuff, I think.

Also, a call to to plperl_call_perl_func should be cast to void to avoid a possible compiler warning (although It doesn't emit one on my system):

(void) plperl_call_perl_func(&desc, &fake_fcinfo);

Right.

I don't get the warning either, and didn't realize it could produce one.
Thanks -- that change is also in the attached version.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

Joshua Tolley wrote:

+ 	plperl_call_data *save_call_data = current_call_data;
+ 	bool		oldcontext = trusted_context;
+ 
+ 	if (SPI_connect() != SPI_OK_CONNECT)
+ 		elog(ERROR, "could not connect to SPI manager");

...

+ 	current_call_data = (plperl_call_data *) palloc0(sizeof(plperl_call_data));
+ 	current_call_data->fcinfo = &fake_fcinfo;
+ 	current_call_data->prodesc = &desc;	

I don't think this is done in the right order. If it is then this
comment in plperl_func_handler is wrong (as well as containing a typo):

/*
* Create the call_data beforing connecting to SPI, so that it is not
* allocated in the SPI memory context
*/

cheers

andrew