Re: BUG #2917: spi_prepare doesn't accept typename aliases

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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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

Andrew Dunstan <andrew(at)dunslane(dot)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(at)nasby(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)nasby(dot)net
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)