Thread: 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
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? > > http://www.postgresql.org/docs/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: DECLAREv 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)