BUG #2917: spi_prepare doesn't accept typename aliases such as 'integer'
The author of this bug was good enough to send me a copy, since I don't
normally read the -bugs list. It can now be found at
http://archives.postgresql.org/pgsql-bugs/2007-01/msg00111.php .
I dug into it a bit and found that pltcl and plpython appear to use
almost identical code, but only pltcl has this limitation documented.
I'm inclined to say we should document this for plperl and plpython for
stable releases and remove the limitation for all three for 8.3. I see
that SQL level prepare calls regprocin() to resolve type names, so maybe
we should that for the PLs when calling SPI_prepare as well.
Alternatively, should we adjust things lower down (e.g. in
typenameType() or LookupTypeName() ) ?
Comments?
cheers
andrew
I wrote:
I see that SQL level prepare calls regprocin() to resolve type names,
so maybe we should that for the PLs when calling SPI_prepare as well.
Of course, that should be regtypein()
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
I see that SQL level prepare calls regprocin() to resolve type names,
so maybe we should that for the PLs when calling SPI_prepare as well.Of course, that should be regtypein()
[ squint... ] build_regtype_array seems a rather stupid bit of code.
How many hundreds of cycles is it expending to convert an OID to an OID?
regards, tom lane
Andrew Dunstan <andrew@dunslane.net> writes:
I dug into it a bit and found that pltcl and plpython appear to use
almost identical code, but only pltcl has this limitation documented.
I'm inclined to say we should document this for plperl and plpython for
stable releases and remove the limitation for all three for 8.3. I see
that SQL level prepare calls regprocin() to resolve type names, so maybe
we should that for the PLs when calling SPI_prepare as well.
I think parseTypeString() may be the thing to use. It's what plpgsql
uses...
regards, tom lane
Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
I see that SQL level prepare calls regprocin() to resolve type names,
so maybe we should that for the PLs when calling SPI_prepare as well.Of course, that should be regtypein()
[ squint... ] build_regtype_array seems a rather stupid bit of code.
How many hundreds of cycles is it expending to convert an OID to an OID?
Yeah, you're right. I should have squinted too ;-)
Well, I am open to suggestions on what to do about this. I really think
we should support use of standard type aliases in the PLs.
cheers
andrew
Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
I dug into it a bit and found that pltcl and plpython appear to use
almost identical code, but only pltcl has this limitation documented.
I'm inclined to say we should document this for plperl and plpython for
stable releases and remove the limitation for all three for 8.3. I see
that SQL level prepare calls regprocin() to resolve type names, so maybe
we should that for the PLs when calling SPI_prepare as well.I think parseTypeString() may be the thing to use. It's what plpgsql
uses...
OK, I'll see what I can do.
cheers
andrew.
I wrote:
Tom Lane wrote:
I think parseTypeString() may be the thing to use. It's what plpgsql
uses...OK, I'll see what I can do.
see attached patch. If this is OK I will apply it and also fix pltcl
and plpython similarly, mutatis mutandis.
cheers
andrew
Attachments:
plperl-prepargs.patchtext/x-patch; name=plperl-prepargs.patchDownload
Index: src/pl/plperl/plperl.c
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plperl/plperl.c,v
retrieving revision 1.123
diff -c -r1.123 plperl.c
*** src/pl/plperl/plperl.c 21 Nov 2006 16:59:02 -0000 1.123
--- src/pl/plperl/plperl.c 26 Jan 2007 15:13:05 -0000
***************
*** 2128,2145 ****
PG_TRY();
{
/************************************************************
! * Lookup the argument types by name in the system cache
! * and remember the required information for input conversion
************************************************************/
for (i = 0; i < argc; i++)
{
! List *names;
HeapTuple typeTup;
! /* Parse possibly-qualified type name and look it up in pg_type */
! names = stringToQualifiedNameList(SvPV(argv[i], PL_na),
! "plperl_spi_prepare");
! typeTup = typenameType(NULL, makeTypeNameFromNameList(names));
qdesc->argtypes[i] = HeapTupleGetOid(typeTup);
perm_fmgr_info(((Form_pg_type) GETSTRUCT(typeTup))->typinput,
&(qdesc->arginfuncs[i]));
--- 2128,2152 ----
PG_TRY();
{
/************************************************************
! * Resolve argument type names and then look them up by oid
! * in the system cache, and remember the required information
! * for input conversion.
************************************************************/
for (i = 0; i < argc; i++)
{
! Oid typeId;
! int32 typmod;
HeapTuple typeTup;
! parseTypeString(SvPV(argv[i], PL_na), &typeId, &typmod);
!
! typeTup = SearchSysCache(TYPEOID,
! ObjectIdGetDatum(typeId),
! 0,0,0);
! if (!HeapTupleIsValid(typeTup))
! elog(ERROR, "cache lookup failed for type %u", typeId);
!
!
qdesc->argtypes[i] = HeapTupleGetOid(typeTup);
perm_fmgr_info(((Form_pg_type) GETSTRUCT(typeTup))->typinput,
&(qdesc->arginfuncs[i]));
Index: src/pl/plperl/expected/plperl.out
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plperl/expected/plperl.out,v
retrieving revision 1.9
diff -c -r1.9 plperl.out
*** src/pl/plperl/expected/plperl.out 13 Aug 2006 17:31:10 -0000 1.9
--- src/pl/plperl/expected/plperl.out 26 Jan 2007 15:13:05 -0000
***************
*** 438,444 ****
-- Test spi_prepare/spi_exec_prepared/spi_freeplan
--
CREATE OR REPLACE FUNCTION perl_spi_prepared(INTEGER) RETURNS INTEGER AS $$
! my $x = spi_prepare('select $1 AS a', 'INT4');
my $q = spi_exec_prepared( $x, $_[0] + 1);
spi_freeplan($x);
return $q->{rows}->[0]->{a};
--- 438,444 ----
-- Test spi_prepare/spi_exec_prepared/spi_freeplan
--
CREATE OR REPLACE FUNCTION perl_spi_prepared(INTEGER) RETURNS INTEGER AS $$
! my $x = spi_prepare('select $1 AS a', 'INTEGER');
my $q = spi_exec_prepared( $x, $_[0] + 1);
spi_freeplan($x);
return $q->{rows}->[0]->{a};
***************
*** 468,470 ****
--- 468,504 ----
4
(2 rows)
+ --
+ -- Test prepare with a type with spaces
+ --
+ CREATE OR REPLACE FUNCTION perl_spi_prepared_double(double precision) RETURNS double precision AS $$
+ my $x = spi_prepare('SELECT 10.0 * $1 AS a', 'DOUBLE PRECISION');
+ my $q = spi_query_prepared($x,$_[0]);
+ my $result;
+ while (defined (my $y = spi_fetchrow($q))) {
+ $result = $y->{a};
+ }
+ spi_freeplan($x);
+ return $result;
+ $$ LANGUAGE plperl;
+ SELECT perl_spi_prepared_double(4.35) as "double precision";
+ double precision
+ ------------------
+ 43.5
+ (1 row)
+
+ --
+ -- Test with a bad type
+ --
+ CREATE OR REPLACE FUNCTION perl_spi_prepared_bad(double precision) RETURNS double precision AS $$
+ my $x = spi_prepare('SELECT 10.0 * $1 AS a', 'does_not_exist');
+ my $q = spi_query_prepared($x,$_[0]);
+ my $result;
+ while (defined (my $y = spi_fetchrow($q))) {
+ $result = $y->{a};
+ }
+ spi_freeplan($x);
+ return $result;
+ $$ LANGUAGE plperl;
+ SELECT perl_spi_prepared_bad(4.35) as "double precision";
+ ERROR: error from Perl function: type "does_not_exist" does not exist at line 2.
Index: src/pl/plperl/sql/plperl.sql
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plperl/sql/plperl.sql,v
retrieving revision 1.11
diff -c -r1.11 plperl.sql
*** src/pl/plperl/sql/plperl.sql 13 Aug 2006 17:31:10 -0000 1.11
--- src/pl/plperl/sql/plperl.sql 26 Jan 2007 15:13:05 -0000
***************
*** 316,322 ****
-- Test spi_prepare/spi_exec_prepared/spi_freeplan
--
CREATE OR REPLACE FUNCTION perl_spi_prepared(INTEGER) RETURNS INTEGER AS $$
! my $x = spi_prepare('select $1 AS a', 'INT4');
my $q = spi_exec_prepared( $x, $_[0] + 1);
spi_freeplan($x);
return $q->{rows}->[0]->{a};
--- 316,322 ----
-- Test spi_prepare/spi_exec_prepared/spi_freeplan
--
CREATE OR REPLACE FUNCTION perl_spi_prepared(INTEGER) RETURNS INTEGER AS $$
! my $x = spi_prepare('select $1 AS a', 'INTEGER');
my $q = spi_exec_prepared( $x, $_[0] + 1);
spi_freeplan($x);
return $q->{rows}->[0]->{a};
***************
*** 337,339 ****
--- 337,369 ----
$$ LANGUAGE plperl;
SELECT * from perl_spi_prepared_set(1,2);
+ --
+ -- Test prepare with a type with spaces
+ --
+ CREATE OR REPLACE FUNCTION perl_spi_prepared_double(double precision) RETURNS double precision AS $$
+ my $x = spi_prepare('SELECT 10.0 * $1 AS a', 'DOUBLE PRECISION');
+ my $q = spi_query_prepared($x,$_[0]);
+ my $result;
+ while (defined (my $y = spi_fetchrow($q))) {
+ $result = $y->{a};
+ }
+ spi_freeplan($x);
+ return $result;
+ $$ LANGUAGE plperl;
+ SELECT perl_spi_prepared_double(4.35) as "double precision";
+
+ --
+ -- Test with a bad type
+ --
+ CREATE OR REPLACE FUNCTION perl_spi_prepared_bad(double precision) RETURNS double precision AS $$
+ my $x = spi_prepare('SELECT 10.0 * $1 AS a', 'does_not_exist');
+ my $q = spi_query_prepared($x,$_[0]);
+ my $result;
+ while (defined (my $y = spi_fetchrow($q))) {
+ $result = $y->{a};
+ }
+ spi_freeplan($x);
+ return $result;
+ $$ LANGUAGE plperl;
+ SELECT perl_spi_prepared_bad(4.35) as "double precision";
+
Andrew Dunstan <andrew@dunslane.net> writes:
see attached patch. If this is OK I will apply it and also fix pltcl
and plpython similarly, mutatis mutandis.
Looks alright as far as it goes, but I'd suggest making one additional
cleanup while you're in there: get rid of the direct syscache access
altogether, instead using getTypeInputInfo(). The loop body should just
consist of three function calls: parseTypeString, getTypeInputInfo,
perm_fmgr_info.
If you wanted to be a bit more ambitious maybe you could change the fact
that this code is throwing away typmod, which means that declarations
like "varchar(32)" would fail to work as expected. Perhaps it should be
fixed to save the typmods alongside the typioparams and then pass them
to InputFunctionCall instead of passing -1. On the other hand, we don't
currently enforce typmod for any function input or result arguments, so
maybe it's consistent that spi_prepare arguments ignore typmods too.
Thoughts?
regards, tom lane
On Jan 26, 2007, at 9:31 AM, Tom Lane wrote:
If you wanted to be a bit more ambitious maybe you could change the
fact
that this code is throwing away typmod, which means that declarations
like "varchar(32)" would fail to work as expected. Perhaps it
should be
fixed to save the typmods alongside the typioparams and then pass them
to InputFunctionCall instead of passing -1. On the other hand, we
don't
currently enforce typmod for any function input or result
arguments, so
maybe it's consistent that spi_prepare arguments ignore typmods too.
Thoughts?
I'd like to see us move towards supporting that; both for function
parameters/results as well as inside functions. It'd be nice if both
cases got fixed at once, but IMHO fixing only one now would be better
than fixing none.
--
Jim Nasby jim@nasby.net
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)
Jim Nasby wrote:
On Jan 26, 2007, at 9:31 AM, Tom Lane wrote:
If you wanted to be a bit more ambitious maybe you could change the fact
that this code is throwing away typmod, which means that declarations
like "varchar(32)" would fail to work as expected. Perhaps it should be
fixed to save the typmods alongside the typioparams and then pass them
to InputFunctionCall instead of passing -1. On the other hand, we don't
currently enforce typmod for any function input or result arguments, so
maybe it's consistent that spi_prepare arguments ignore typmods too.
Thoughts?I'd like to see us move towards supporting that; both for function
parameters/results as well as inside functions. It'd be nice if both
cases got fixed at once, but IMHO fixing only one now would be better
than fixing none.
I'm not going to do either in fixing this bug - I think they should be
fixed but are a separate issue. These probably belong on the TODO list.
cheers
andrew
OK, what is the TODO wording?
---------------------------------------------------------------------------
Andrew Dunstan wrote:
Jim Nasby wrote:
On Jan 26, 2007, at 9:31 AM, Tom Lane wrote:
If you wanted to be a bit more ambitious maybe you could change the fact
that this code is throwing away typmod, which means that declarations
like "varchar(32)" would fail to work as expected. Perhaps it should be
fixed to save the typmods alongside the typioparams and then pass them
to InputFunctionCall instead of passing -1. On the other hand, we don't
currently enforce typmod for any function input or result arguments, so
maybe it's consistent that spi_prepare arguments ignore typmods too.
Thoughts?I'd like to see us move towards supporting that; both for function
parameters/results as well as inside functions. It'd be nice if both
cases got fixed at once, but IMHO fixing only one now would be better
than fixing none.I'm not going to do either in fixing this bug - I think they should be
fixed but are a separate issue. These probably belong on the TODO list.cheers
andrew
---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote:
OK, what is the TODO wording?cheers
Something like:
Enforce typmod for function inputs, function results and parameters for
spi_prepare'd statements called from PLs.
cheers
andrew
Andrew Dunstan wrote:
Bruce Momjian wrote:
OK, what is the TODO wording?cheers
Something like:
Enforce typmod for function inputs, function results and parameters for
spi_prepare'd statements called from PLs.
Added.
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
What about plpgsql variables, ie:
DECLARE
v varchar(42);
BEGIN
...
On Jan 26, 2007, at 9:48 PM, Andrew Dunstan wrote:
Bruce Momjian wrote:
OK, what is the TODO wording?cheers
Something like:
Enforce typmod for function inputs, function results and parameters
for spi_prepare'd statements called from PLs.cheers
andrew
--
Jim Nasby jim@nasby.net
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)