Re: [PATCHES] latest plperl

[Joe Conway]

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
---(end of broadcast)---
TIP 6: Have you searched our list archives?
  http://archives.postgresql.org

Re: [PATCHES] latest plperl

[Joe Conway]

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
Index: src/pl/plperl/GNUmakefile
===
RCS file: /cvsroot/pgsql-server/src/pl/plperl/GNUmakefile,v
retrieving revision 1.12
diff -c -r1.12 GNUmakefile
*** src/pl/plperl/GNUmakefile	21 Jan 2004 19:04:11 -	1.12
--- src/pl/plperl/GNUmakefile	1 Jul 2004 16:24:53 -
***
*** 25,32 
  SO_MAJOR_VERSION = 0
  SO_MINOR_VERSION = 0
  
! OBJS = plperl.o eloglvl.o SPI.o
  SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS)
  
  include $(top_srcdir)/src/Makefile.shlib
  
--- 25,37 
  SO_MAJOR_VERSION = 0
  SO_MINOR_VERSION = 0
  
! OBJS = plperl.o spi_internal.o SPI.o
! 
! ifeq ($(enable_rpath), yes)
  SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS)
+ else
+ SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS) -Wl,-rpath,$(perl_archlibexp)/CORE
+ endif
  
  include $(top_srcdir)/src/Makefile.shlib
  
Index: src/pl/plperl/SPI.xs
===
RCS file: /cvsroot/pgsql-server/src/pl/plperl/SPI.xs,v
retrieving revision 1.5
diff -c -r1.5 SPI.xs
*** src/pl/plperl/SPI.xs	4 Sep 2002 22:49:37 -	1.5
--- src/pl/plperl/SPI.xs	1 Jul 2004 16:24:53 -
***
*** 6,22 
  #include perl.h
  #include XSUB.h
  
! #include eloglvl.h
  
  
  
! MODULE = SPI PREFIX = elog_
  
  PROTOTYPES: ENABLE
  VERSIONCHECK: DISABLE
  
  void
! elog_elog(level, message)
  	int level
  	char* message
  	CODE:
--- 6,22 
  #include perl.h
  #include XSUB.h
  
! #include spi_internal.h
  
  
  
! MODULE = SPI PREFIX = spi_
  
  PROTOTYPES: ENABLE
  VERSIONCHECK: DISABLE
  
  void
! spi_elog(level, message)
  	int level
  	char* message
  	CODE:
***
*** 24,44 
  
  
  int
! elog_DEBUG()
  
  int
! elog_LOG()
  
  int
! elog_INFO()
  
  int
! elog_NOTICE()
  
  int
! elog_WARNING()
  
  int
! elog_ERROR()
! 
  
--- 24,56 
  
  
  int
! spi_DEBUG()
  
  int
! spi_LOG()
  
  int
! spi_INFO()
  
  int
! spi_NOTICE()
  
  int
! spi_WARNING()
  
  int
! spi_ERROR()
  
+ SV*
+ spi_spi_exec_query(query, ...)
+ 	char* query;
+ 	PREINIT:
+ 		HV *ret_hash;
+ 		int limit=0;
+ 	CODE:
+ 			if (items2) Perl_croak(aTHX_ Usage: spi_exec_query(query, limit) or spi_exec_query(query));
+ 			if (items == 2) limit = SvIV(ST(1));
+ 			ret_hash=plperl_spi_exec(query, limit);
+ 		RETVAL = newRV_noinc((SV*)ret_hash);
+ 	OUTPUT:
+ 		RETVAL
Index: src/pl/plperl/eloglvl.c
===
RCS file: src/pl/plperl/eloglvl.c
diff -N src/pl/plperl/eloglvl.c
*** src/pl/plperl/eloglvl.c	25 Jul 2003 23:37:28 -	1.9
--- /dev/null	1 Jan 1970 00:00:00 -
***
*** 1,45 
- #include postgres.h
- 
- /*
-  * This kludge is necessary because of the conflicting
-  * definitions of 'DEBUG' between postgres and perl.
-  * we'll live.
-  */
- 
- #include eloglvl.h
- 
- int
- elog_DEBUG(void)
- {
- 	return DEBUG2;
- }
- 
- int
- elog_LOG(void)
- {
- 	return LOG;
- }
- 
- int
- elog_INFO(void)
- {
- 	return INFO;
- }
- 
- int
- elog_NOTICE(void)
- {
- 	return NOTICE;
- }
- 
- int
- elog_WARNING(void)
- {
- 	return WARNING;
- }
- 
- int
- elog_ERROR(void)
- {
- 	return ERROR;
- }
--- 0 
Index: src/pl/plperl/eloglvl.h
===
RCS file: src/pl/plperl/eloglvl.h
diff -N src/pl/plperl/eloglvl.h
*** src/pl/plperl/eloglvl.h	4 Sep 2002 20:31:47 -	1.5
--- /dev/null	1 Jan 1970 00:00:00 -
***
*** 1,12 
- 
- int			elog_DEBUG(void);
- 
- int			elog_LOG(void);
- 
- int			elog_INFO(void);
- 
- int			elog_NOTICE(void);
- 
- int			elog_WARNING(void);
- 
- int			elog_ERROR(void);
--- 0 
Index: src/pl/plperl/plperl.c
===
RCS file: /cvsroot/pgsql-server/src/pl/plperl/plperl.c,v
retrieving revision 1.44
diff -c -r1.44 plperl.c
*** src/pl/plperl/plperl.c	6 Jun 2004 00:41:28 -	1.44
--- src/pl/plperl/plperl.c	1 Jul 2004 16:24:53 -
***
*** 49,54 
--- 49,55 
  #include catalog/pg_language.h
  #include catalog/pg_proc.h
  #include catalog/pg_type.h
+ #include funcapi.h			/* need for SRF support */
  #include commands/trigger.h
  #include executor/spi.h
  #include fmgr.h
***
*** 78,83 
--- 79,86 
  	TransactionId fn_xmin;
  	CommandId	fn_cmin;
  	bool		lanpltrusted;
+ 	bool		fn_retistuple;	/* true, if function returns tuple */
+ 	Oid			ret_oid;		/* Oid of returning type */
  	FmgrInfo	

Re: [PATCHES] latest plperl

[Joe Conway]

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
---(end of broadcast)---
TIP 3: if posting/reading through Usenet, please send an appropriate
 subscribe-nomail command to [EMAIL PROTECTED] so that your
 message can get through to the mailing list cleanly

Re: [PATCHES] latest plperl

[Tom Lane]

Joe Conway [EMAIL PROTECTED] 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

---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster

Re: [PATCHES] latest plperl

[Andrew Dunstan]

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




---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]

Re: [PATCHES] latest plperl

[Alvaro Herrera]

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)


---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]

Re: [PATCHES] latest plperl

[Joe Conway]

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
---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings

[PATCHES] latest plperl

[Andrew Dunstan]

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.

There are no outstanding issues I am aware of.

The previously advised limitation on representation of arrays and embedded
composites remains, at least for now.

We have started work on a doc set.

cheers

andrew




plperl.patch
Description: Binary data
#include postgres.h
#include executor/spi.h
#include utils/syscache.h
/*
 * This kludge is necessary because of the conflicting
 * definitions of 'DEBUG' between postgres and perl.
 * we'll live.
 */

#include spi_internal.h

static char* plperl_spi_status_string(int);

static HV* plperl_spi_execute_fetch_result(SPITupleTable*, int, int );

int
spi_DEBUG(void)
{
	return DEBUG2;
}

int
spi_LOG(void)
{
	return LOG;
}

int
spi_INFO(void)
{
	return INFO;
}

int
spi_NOTICE(void)
{
	return NOTICE;
}

int
spi_WARNING(void)
{
	return WARNING;
}

int
spi_ERROR(void)
{
	return ERROR;
}

HV*
plperl_spi_exec(char* query, int limit)
{
	HV *ret_hv;
	int spi_rv;

	spi_rv = SPI_exec(query, limit);
	ret_hv=plperl_spi_execute_fetch_result(SPI_tuptable, SPI_processed, spi_rv);

	return ret_hv;
}

static HV*
plperl_hash_from_tuple(HeapTuple tuple, TupleDesc tupdesc)
{
	int	i;
	char	*attname;
	char	*attdata;

	HV *array;

	array = newHV();

	for (i = 0; i  tupdesc-natts; i++) {
		/
		* Get the attribute name
		/
		attname = tupdesc-attrs[i]-attname.data;

		/
		* Get the attributes value
		/
		attdata = SPI_getvalue(tuple, tupdesc, i+1);
		hv_store(array, attname, strlen(attname), newSVpv(attdata,0), 0);
	}
	return array;
}

static HV*
plperl_spi_execute_fetch_result(SPITupleTable *tuptable, int rows, int status)
{

	HV *result;
	int i;

	result = newHV();

	if (status == SPI_OK_UTILITY)
	{
		hv_store(result, status, strlen(status), newSVpv(SPI_OK_UTILITY,0), 0);
		hv_store(result, rows, strlen(rows), newSViv(rows), 0);
	}
	else if (status != SPI_OK_SELECT)
	{
		hv_store(result, status, strlen(status), newSVpv((char*)plperl_spi_status_string(status),0), 0);
		hv_store(result, rows, strlen(rows), newSViv(rows), 0);
	}
	else
	{
		if (rows)
		{
			char* key=palloc(sizeof(int));
			HV *row;
			for (i = 0; i  rows; i++)
			{
row = plperl_hash_from_tuple(tuptable-vals[i], tuptable-tupdesc);
sprintf(key, %i, i);
hv_store(result, key, strlen(key), newRV_noinc((SV*)row), 0);
			}
			SPI_freetuptable(tuptable);
		}
	}
	return result;
}

static char*
plperl_spi_status_string(int status)
{
	switch(status){
		/*errors*/
		case SPI_ERROR_TYPUNKNOWN:
			return SPI_ERROR_TYPUNKNOWN;
		case SPI_ERROR_NOOUTFUNC:
			return SPI_ERROR_NOOUTFUNC;
		case SPI_ERROR_NOATTRIBUTE:
			return SPI_ERROR_NOATTRIBUTE;
		case SPI_ERROR_TRANSACTION:
			return SPI_ERROR_TRANSACTION;
		case SPI_ERROR_PARAM:
			return SPI_ERROR_PARAM;
		case SPI_ERROR_ARGUMENT:
			return SPI_ERROR_ARGUMENT;
		case SPI_ERROR_CURSOR:
			return SPI_ERROR_CURSOR;
		case SPI_ERROR_UNCONNECTED:
			return SPI_ERROR_UNCONNECTED;
		case SPI_ERROR_OPUNKNOWN:
			return SPI_ERROR_OPUNKNOWN;
		case SPI_ERROR_COPY:
			return SPI_ERROR_COPY;
		case SPI_ERROR_CONNECT:
			return SPI_ERROR_CONNECT;
		/*ok*/
		case SPI_OK_CONNECT:
			return SPI_OK_CONNECT;
		case SPI_OK_FINISH:
			return SPI_OK_FINISH;
		case SPI_OK_FETCH:
			return SPI_OK_FETCH;
		case SPI_OK_UTILITY:
			return SPI_OK_UTILITY;
		case SPI_OK_SELECT:
			return SPI_OK_SELECT;
		case SPI_OK_SELINTO:
			return SPI_OK_SELINTO;
		case SPI_OK_INSERT:
			return SPI_OK_INSERT;
		case SPI_OK_DELETE:
			return SPI_OK_DELETE;
		case SPI_OK_UPDATE:
			return SPI_OK_UPDATE;
		case SPI_OK_CURSOR:
			return SPI_OK_CURSOR;
	}

	return Unknown or Invalid code;
}#include EXTERN.h
#include perl.h
#include XSUB.h

int			spi_DEBUG(void);

int			spi_LOG(void);

int			spi_INFO(void);

int			spi_NOTICE(void);

int			spi_WARNING(void);

int			spi_ERROR(void);

HV*		plperl_spi_exec(char*, int);



---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster

Re: [PATCHES] latest plperl

[Joe Conway]

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
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]

Re: [PATCHES] latest plperl

[Andrew Dunstan]

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.2content-type=text/x-cvsweb-markup 
that can be used as a starting point. Any contributions welcome.

cheers
andrew


---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]

Re: [PATCHES] latest plperl

[Tom Lane]

Joe Conway [EMAIL PROTECTED] 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

---(end of broadcast)---
TIP 9: the planner will ignore your desire to choose an index scan if your
  joining column's datatypes do not match

Re: [PATCHES] latest plperl

[Joe Conway]

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
---(end of broadcast)---
TIP 8: explain analyze is your friend

Re: [PATCHES] latest plperl

[Tom Lane]

Joe Conway [EMAIL PROTECTED] 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

---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster

Re: [PATCHES] latest plperl

[Andrew Dunstan]

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




---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings

Re: [PATCHES] latest plperl

[Joe Conway]

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
---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster