plpgsql doesn't supply typmod for the Params it generates

pl_comp.c does this to set up Params representing values it supplies for
expression/query execution:

param = makeNode(Param);
param->paramkind = PARAM_EXTERN;
param->paramid = dno + 1;
param->paramtype = exec_get_datum_type(estate, datum);
param->paramtypmod = -1;
param->paramcollid = exec_get_datum_collation(estate, datum);
param->location = location;

On reflection it seems a bit silly to not supply typmod: we know a
typmod for each plpgsql variable or record field, so why not provide it?

This omission is the cause of bug #6020:
http://archives.postgresql.org/pgsql-bugs/2011-05/msg00080.php

The submitter's example can be boiled down to this self-contained case:

CREATE OR REPLACE FUNCTION add_new_card()
RETURNS record AS
$BODY$
DECLARE
new_card_id INTEGER;
magic_byte CHAR(8);
crazy_eights CHAR(8);
return_pieces RECORD;
BEGIN
new_card_id := 42;
magic_byte := '12345678';
crazy_eights := 'abcd1234';

return_pieces := (new_card_id, magic_byte, crazy_eights);
RETURN return_pieces;
END
$BODY$
LANGUAGE plpgsql;

SELECT * FROM
add_new_card() AS (id INTEGER, magic CHAR(8), crazy CHAR(8));

Note that the reason this code broke in 9.0 is not that 9.0 got dumber
about supplying typmod data for plpgsql variables --- we never did that
before, which is why the Params are being set up without it. Rather,
it's that 9.0 is actually checking for typmod match where previous
releases didn't. If you increase the length of magic_byte in the above
example, pre-9.0 happily returns data that violates the length
constraint shown in the outer query.

Another consideration here is that because I back-ported the 9.0
checking code into REL8_4:
http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=5d3853a7fa40b28b44b14084863fd83a188c9a9e
8.4.8 in fact behaves like 9.0, which would be a regression for somebody
who relies on code like this to work.

I think the appropriate fix is pretty clear: add a function similar to
exec_get_datum_type that returns the datum's typmod, and use that to set
paramtypmod properly. What is worrying me is that it's not clear how
much user-visible behavioral change will result, and therefore I'm not
entirely comfortable with the idea of back-patching such a change into
9.0 --- and it wouldn't work at all in 8.4, where there simply isn't a
good opportunity to set a typmod for parameters passed to the main
executor (since the SPI interfaces plpgsql uses don't support that).

I don't especially want to revert the above-mentioned 8.4 commit
altogether, but maybe we should consider tweaking its version of
tupconvert.c to not check typmods? All the alternatives there seem
unpalatable.

Any opinions what to do?

regards, tom lane

I wrote:
> I think the appropriate fix is pretty clear: add a function similar to
> exec_get_datum_type that returns the datum's typmod, and use that to set
> paramtypmod properly. What is worrying me is that it's not clear how
> much user-visible behavioral change will result, and therefore I'm not
> entirely comfortable with the idea of back-patching such a change into
> 9.0 --- and it wouldn't work at all in 8.4, where there simply isn't a
> good opportunity to set a typmod for parameters passed to the main
> executor (since the SPI interfaces plpgsql uses don't support that).

Attached is a proposed patch for HEAD that sets up the Param's typmod
sanely. I've verified that this fixes the reported problem and does not
result in any changes in the regression tests, which makes me a bit more
optimistic about it ... but I'm still not convinced it'd be a good idea
to back-patch into 9.0.

regards, tom lane

On Thu, May 12, 2011 at 5:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> I think the appropriate fix is pretty clear: add a function similar to
>> exec_get_datum_type that returns the datum's typmod, and use that to set
>> paramtypmod properly.  What is worrying me is that it's not clear how
>> much user-visible behavioral change will result, and therefore I'm not
>> entirely comfortable with the idea of back-patching such a change into
>> 9.0 --- and it wouldn't work at all in 8.4, where there simply isn't a
>> good opportunity to set a typmod for parameters passed to the main
>> executor (since the SPI interfaces plpgsql uses don't support that).
>
> Attached is a proposed patch for HEAD that sets up the Param's typmod
> sanely.  I've verified that this fixes the reported problem and does not
> result in any changes in the regression tests, which makes me a bit more
> optimistic about it ... but I'm still not convinced it'd be a good idea
> to back-patch into 9.0.

Back-patching doesn't seem very safe to me, either; though I'm not
entirely certain what to do instead. Relaxing the check, as you
proposed upthread, might be the way to go.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company