latest plperl

Andrew Dunstan wrote:

The attached patch (and 2 new files incorporating previous eloglvl.[ch] as
before) has the following changes over previously sent patch
(fixes all by me):

- fix null <-> undef mappings
- fix GNUmakefile to honor rpath configuration, and remove ugly compile
arnings due to inappropriate use of rpath in CFLAGS
- very minor code comment cleanup

The feature set is as previously advised.

I've been working with Andrew and company on this for a few days. I
intend to finish up my code review and commit it tomorrow sometime,
unless someone has objections.

That said, I'm not particularly strong in perl, so it would be helpful
if others would test and report in.

Thanks,

Joe

Joe Conway wrote:

Andrew Dunstan wrote:

The attached patch (and 2 new files incorporating previous
eloglvl.[ch] as
before) has the following changes over previously sent patch
(fixes all by me):

- fix null <-> undef mappings
- fix GNUmakefile to honor rpath configuration, and remove ugly compile
arnings due to inappropriate use of rpath in CFLAGS
- very minor code comment cleanup

The feature set is as previously advised.

I've been working with Andrew and company on this for a few days. I
intend to finish up my code review and commit it tomorrow sometime,
unless someone has objections.

That said, I'm not particularly strong in perl, so it would be helpful
if others would test and report in.

Thanks, Joe.

There is a very small test script here:
http://cvs.pgfoundry.org/cgi-bin/cvsweb.cgi/plperlng/plperlng/plperl-test.sql?rev=1.2&amp;content-type=text/x-cvsweb-markup
that can be used as a starting point. Any contributions welcome.

cheers

andrew

Joe Conway <mail@joeconway.com> writes:

I've been working with Andrew and company on this for a few days. I
intend to finish up my code review and commit it tomorrow sometime,
unless someone has objections.

Oh good. I've been feeling stretched a bit thin --- if you want to deal
with the plperl patch it's fine with me. Are there any other pending
patches you're interested in taking responsibility for?

regards, tom lane

Tom Lane wrote:

Are there any other pending patches you're interested in taking
responsibility for?

Yeah, I know you've been especially overloaded lately, and I feel badly
that I've not been able to help out in recent months :-(

If you have some specific patches in mind, I can try to work on one or
more tomorrow and Friday. Unfortunately, on Saturday morning I'm leaving
on a 3600 mile roadtrip by car, and while I'm gone my connectivity will
be spotty (for a week and a half).

Joe

Andrew Dunstan wrote:

The attached patch (and 2 new files incorporating previous eloglvl.[ch] as
before) has the following changes over previously sent patch
(fixes all by me):

The patch file itself seems to be empty -- please resend.

Thanks,

Joe

Joe Conway <mail@joeconway.com> writes:

If you have some specific patches in mind, I can try to work on one or
more tomorrow and Friday. Unfortunately, on Saturday morning I'm leaving
on a 3600 mile roadtrip by car, and while I'm gone my connectivity will
be spotty (for a week and a half).

Fair enough --- I'm taking next week off too. Looks like it'll be
Bruce's problem ;-)

regards, tom lane

Joe Conway said:

Andrew Dunstan wrote:

The attached patch (and 2 new files incorporating previous
eloglvl.[ch] as before) has the following changes over previously
sent patch
(fixes all by me):

The patch file itself seems to be empty -- please resend.

it has 36k with expected contents in my mailbox.

cheers

andrew

Andrew Dunstan wrote:

The attached patch (and 2 new files incorporating previous eloglvl.[ch] as
before) has the following changes over previously sent patch
(fixes all by me):

Some comments below:

--------------------
In plperl_trigger_build_args(), this looks bogus:

+ 	char	   *tmp;
+
+ 	tmp = (char *) malloc(sizeof(int));
...
+ 	sprintf(tmp, "%d", tdata->tg_trigger->tgnargs);
+ 	sv_catpvf(rv, ", argc => %s", tmp);
...
+ 	free(tmp);

I changed it to:

+ sv_catpvf(rv, ", argc => %d", tdata->tg_trigger->tgnargs);

--------------------
In this section, it appears that empty strings in the tuple will be
coerced into NULL values:

+ plval = plperl_get_elem(hvNew, platt);

+         if (plval)
+         {
+             src = plval;
+             if (strlen(plval))
+             {
+                 modvalues[j] = FunctionCall3(&finfo,
+                       CStringGetDatum(src),
+                       ObjectIdGetDatum(typelem),
+                       Int32GetDatum(tupdesc->attrs[atti]->atttypmod));
+                 modnulls[j] = ' ';
+             }
+             else
+             {
+                 modvalues[i] = (Datum) 0;
+                 modnulls[j] = 'n';
+             }
+         }
+         plval = NULL;

Shouldn't that look more like this?

+ plval = plperl_get_elem(hvNew, platt);

+         if (plval)
+         {
+             modvalues[j] = FunctionCall3(&finfo,
+                   CStringGetDatum(plval),
+                   ObjectIdGetDatum(typelem),
+                   Int32GetDatum(tupdesc->attrs[atti]->atttypmod));
+             modnulls[j] = ' ';
+         }
+         else
+         {
+             modvalues[i] = (Datum) 0;
+             modnulls[j] = 'n';
+         }

Joe

Joe Conway said:

Andrew Dunstan wrote:

The attached patch (and 2 new files incorporating previous
eloglvl.[ch] as before) has the following changes over previously
sent patch
(fixes all by me):

Some comments below:

--------------------
In plperl_trigger_build_args(), this looks bogus:

+ 	char	   *tmp;
+
+ 	tmp = (char *) malloc(sizeof(int));
...
+ 	sprintf(tmp, "%d", tdata->tg_trigger->tgnargs);
+ 	sv_catpvf(rv, ", argc => %s", tmp);
...
+ 	free(tmp);

Doh! Very bogus! sizeof(int)and a malloc to boot ???

I didn't check the trigger code much because it has supposedly been working
for quite a while. I will examine more closely.

I changed it to:

+ sv_catpvf(rv, ", argc => %d", tdata->tg_trigger->tgnargs);

works for me.

--------------------
In this section, it appears that empty strings in the tuple will be
coerced into NULL values:

+ plval = plperl_get_elem(hvNew, platt);

+         if (plval)
+         {
+             src = plval;
+             if (strlen(plval))
+             {
+                 modvalues[j] = FunctionCall3(&finfo,
+                       CStringGetDatum(src),
+                       ObjectIdGetDatum(typelem),
+
Int32GetDatum(tupdesc->attrs[atti]->atttypmod)); +
modnulls[j] = ' ';
+             }
+             else
+             {
+                 modvalues[i] = (Datum) 0;
+                 modnulls[j] = 'n';
+             }
+         }
+         plval = NULL;

Shouldn't that look more like this?

+ plval = plperl_get_elem(hvNew, platt);

+         if (plval)
+         {
+             modvalues[j] = FunctionCall3(&finfo,
+                   CStringGetDatum(plval),
+                   ObjectIdGetDatum(typelem),
+                   Int32GetDatum(tupdesc->attrs[atti]->atttypmod)); +
modnulls[j] = ' ';
+         }
+         else
+         {
+             modvalues[i] = (Datum) 0;
+             modnulls[j] = 'n';
+         }

Yes, except that that [i] looks wrong too. Surely it should be [j]. And with
this change decl of src appears redundant.

I will do some checking on these changes, but with those caveats they look
good to me.

Do you need a revised patch?

cheers

andrew

I also got the rpath test sense wrong in the make file fix. It should read
(assuming this mailer dowsn't break lines badly):

ifeq ($(enable_rpath), yes)
SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS)
-Wl,-rpath,$(perl_archlibexp)/CORE
else
SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS)
endif

Please adjust.

thanks

andrew

Andrew Dunstan wrote:

Doh! Very bogus! sizeof(int)and a malloc to boot ???

I didn't check the trigger code much because it has supposedly been working
for quite a while. I will examine more closely.

Well, essentially 4 bytes (sizeof(int)) were being allocated to print a
two byte interger that can never be greater than two characters ("32"),
so I don't expect it would have ever failed, but only by serendipity.

Shouldn't that look more like this?

+ plval = plperl_get_elem(hvNew, platt);

+         if (plval)
+         {
+             modvalues[j] = FunctionCall3(&finfo,
+                   CStringGetDatum(plval),
+                   ObjectIdGetDatum(typelem),
+                   Int32GetDatum(tupdesc->attrs[atti]->atttypmod)); +
modnulls[j] = ' ';
+         }
+         else
+         {
+             modvalues[i] = (Datum) 0;
+             modnulls[j] = 'n';
+         }

Yes, except that that [i] looks wrong too. Surely it should be [j]. And with
this change decl of src appears redundant.

Hmmm, I missed that -- looks wrong to me too. I'll check it out.

I will do some checking on these changes, but with those caveats they look
good to me.

Do you need a revised patch?

Nah, I'll make the changes and post a revised patch for you to comment
on prior to committing.

Joe

Andrew Dunstan wrote:

I will do some checking on these changes, but with those caveats they look
good to me.

Attached is an all inclusive revised patch. Please review and comment.
If there are no objections, I'll commit in a few hours.

As a side note, I think it would be *really* helpful if there were a
more comprehensive test script, and an expected results file available.
Not sure though if it could be included in the standard regression tests
on a configure-conditional basis -- anyone know?

Joe

Andrew Dunstan wrote:

I also got the rpath test sense wrong in the make file fix. It should read
(assuming this mailer dowsn't break lines badly):

ifeq ($(enable_rpath), yes)
SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS)
-Wl,-rpath,$(perl_archlibexp)/CORE
else
SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS)
endif

OK -- wasn't in the last patch I posted, but I got it now.

Joe

Joe Conway said:

As a side note, I think it would be *really* helpful if there were a
more comprehensive test script, and an expected results file available.
Not sure though if it could be included in the standard regression
tests on a configure-conditional basis -- anyone know?

To the best of my knowledge you cannot. We will provide an analogue for
pltcl's test directory shortly, if that is desired - it will probably take a
few days, though. I assume that will be acceptable after feature freeze?

At a quick glance, modulo the makefile change, the patch looks good.

cheers

andrew

Joe Conway <mail@joeconway.com> writes:

As a side note, I think it would be *really* helpful if there were a
more comprehensive test script, and an expected results file available.
Not sure though if it could be included in the standard regression tests
on a configure-conditional basis -- anyone know?

pltcl has a separate regression test, which seems to serve the purpose
well enough. I'd suggest focusing more on getting the tests into
existence than on whether they have to be integrated ;-)

regards, tom lane

On Thu, Jul 01, 2004 at 09:33:57AM -0700, Joe Conway wrote:

As a side note, I think it would be *really* helpful if there were a
more comprehensive test script, and an expected results file available.
Not sure though if it could be included in the standard regression tests
on a configure-conditional basis -- anyone know?

Can't this stuff be tested somehow using Test::Simple, Test::Harness or
something like that? I know this is not standard perl stuff but ...

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"I call it GNU/Linux. Except the GNU/ is silent." (Ben Reiter)

Alvaro Herrera said:

On Thu, Jul 01, 2004 at 09:33:57AM -0700, Joe Conway wrote:

As a side note, I think it would be *really* helpful if there were a
more comprehensive test script, and an expected results file
available. Not sure though if it could be included in the standard
regression tests on a configure-conditional basis -- anyone know?

Can't this stuff be tested somehow using Test::Simple, Test::Harness or
something like that? I know this is not standard perl stuff but ...

Not really. These subroutines have no names nor even references from Perl's
POV. I really don't want to build a test harness into the dynamic lib. And
in any case, the test really needs to come from postgres, not from perl. The
test is 'does this trigger/function do the right thing in the sense of the
value returned to postgres or effect on the database?' The place where
things are likely to break is not in the perl interpreter, but in the glue
code.
cheers

andrew

Andrew Dunstan wrote:

Joe Conway said:

As a side note, I think it would be *really* helpful if there were a
more comprehensive test script, and an expected results file available.
Not sure though if it could be included in the standard regression
tests on a configure-conditional basis -- anyone know?

To the best of my knowledge you cannot. We will provide an analogue for
pltcl's test directory shortly, if that is desired - it will probably take a
few days, though. I assume that will be acceptable after feature freeze?

Yup, I think that falls into Tom's "loose ends" category.

At a quick glance, modulo the makefile change, the patch looks good.

Great! I'll commit shortly.

Thanks,

Joe