Thread: 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

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 535fea9257c22e5cc54dc956ebd59a6865340a55..a80235cd2aaf76e493f125c80a15bae9bc1b11ce 100644
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
*************** make_datum_param(PLpgSQL_expr *expr, int
*** 1267,1275 ****
      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;

      return (Node *) param;
--- 1267,1277 ----
      param = makeNode(Param);
      param->paramkind = PARAM_EXTERN;
      param->paramid = dno + 1;
!     exec_get_datum_type_info(estate,
!                              datum,
!                              ¶m->paramtype,
!                              ¶m->paramtypmod,
!                              ¶m->paramcollid);
      param->location = location;

      return (Node *) param;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 1da5095507468c5c5c81fd0042e69a41edcef304..2a536b688d3efbdbde54d2e010cb706bb3fcab1f 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** exec_get_datum_type(PLpgSQL_execstate *e
*** 4307,4335 ****
  }

  /*
!  * exec_get_datum_collation                Get collation of a PLpgSQL_datum
   */
! Oid
! exec_get_datum_collation(PLpgSQL_execstate *estate,
!                          PLpgSQL_datum *datum)
  {
-     Oid            collid;
-
      switch (datum->dtype)
      {
          case PLPGSQL_DTYPE_VAR:
              {
                  PLpgSQL_var *var = (PLpgSQL_var *) datum;

!                 collid = var->datatype->collation;
                  break;
              }

          case PLPGSQL_DTYPE_ROW:
          case PLPGSQL_DTYPE_REC:
!             /* composite types are never collatable */
!             collid = InvalidOid;
!             break;

          case PLPGSQL_DTYPE_RECFIELD:
              {
--- 4307,4369 ----
  }

  /*
!  * exec_get_datum_type_info            Get datatype etc of a PLpgSQL_datum
!  *
!  * An extended version of exec_get_datum_type, which also retrieves the
!  * typmod and collation of the datum.
   */
! void
! exec_get_datum_type_info(PLpgSQL_execstate *estate,
!                          PLpgSQL_datum *datum,
!                          Oid *typeid, int32 *typmod, Oid *collation)
  {
      switch (datum->dtype)
      {
          case PLPGSQL_DTYPE_VAR:
              {
                  PLpgSQL_var *var = (PLpgSQL_var *) datum;

!                 *typeid = var->datatype->typoid;
!                 *typmod = var->datatype->atttypmod;
!                 *collation = var->datatype->collation;
                  break;
              }

          case PLPGSQL_DTYPE_ROW:
+             {
+                 PLpgSQL_row *row = (PLpgSQL_row *) datum;
+
+                 if (!row->rowtupdesc)    /* should not happen */
+                     elog(ERROR, "row variable has no tupdesc");
+                 /* Make sure we have a valid type/typmod setting */
+                 BlessTupleDesc(row->rowtupdesc);
+                 *typeid = row->rowtupdesc->tdtypeid;
+                 /* do NOT return the mutable typmod of a RECORD variable */
+                 *typmod = -1;
+                 /* composite types are never collatable */
+                 *collation = InvalidOid;
+                 break;
+             }
+
          case PLPGSQL_DTYPE_REC:
!             {
!                 PLpgSQL_rec *rec = (PLpgSQL_rec *) datum;
!
!                 if (rec->tupdesc == NULL)
!                     ereport(ERROR,
!                           (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
!                            errmsg("record \"%s\" is not assigned yet",
!                                   rec->refname),
!                            errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
!                 /* Make sure we have a valid type/typmod setting */
!                 BlessTupleDesc(rec->tupdesc);
!                 *typeid = rec->tupdesc->tdtypeid;
!                 /* do NOT return the mutable typmod of a RECORD variable */
!                 *typmod = -1;
!                 /* composite types are never collatable */
!                 *collation = InvalidOid;
!                 break;
!             }

          case PLPGSQL_DTYPE_RECFIELD:
              {
*************** exec_get_datum_collation(PLpgSQL_execsta
*** 4350,4370 ****
                              (errcode(ERRCODE_UNDEFINED_COLUMN),
                               errmsg("record \"%s\" has no field \"%s\"",
                                      rec->refname, recfield->fieldname)));
!                 /* XXX there's no SPI_getcollid, as yet */
                  if (fno > 0)
!                     collid = rec->tupdesc->attrs[fno - 1]->attcollation;
                  else    /* no system column types have collation */
!                     collid = InvalidOid;
                  break;
              }

          default:
              elog(ERROR, "unrecognized dtype: %d", datum->dtype);
!             collid = InvalidOid;    /* keep compiler quiet */
              break;
      }
-
-     return collid;
  }

  /* ----------
--- 4384,4410 ----
                              (errcode(ERRCODE_UNDEFINED_COLUMN),
                               errmsg("record \"%s\" has no field \"%s\"",
                                      rec->refname, recfield->fieldname)));
!                 *typeid = SPI_gettypeid(rec->tupdesc, fno);
!                 /* XXX there's no SPI_gettypmod, for some reason */
                  if (fno > 0)
!                     *typmod = rec->tupdesc->attrs[fno - 1]->atttypmod;
!                 else
!                     *typmod = -1;
!                 /* XXX there's no SPI_getcollation either */
!                 if (fno > 0)
!                     *collation = rec->tupdesc->attrs[fno - 1]->attcollation;
                  else    /* no system column types have collation */
!                     *collation = InvalidOid;
                  break;
              }

          default:
              elog(ERROR, "unrecognized dtype: %d", datum->dtype);
!             *typeid = InvalidOid;    /* keep compiler quiet */
!             *typmod = -1;
!             *collation = InvalidOid;
              break;
      }
  }

  /* ----------
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index e50d7eb45fedec49a0f675fcd19359100fcc314b..89103aea8c5d3fa967ff2ab4a23105039dfe3a8d 100644
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
*************** extern void plpgsql_subxact_cb(SubXactEv
*** 905,912 ****
                     SubTransactionId parentSubid, void *arg);
  extern Oid exec_get_datum_type(PLpgSQL_execstate *estate,
                      PLpgSQL_datum *datum);
! extern Oid exec_get_datum_collation(PLpgSQL_execstate *estate,
!                          PLpgSQL_datum *datum);

  /* ----------
   * Functions for namespace handling in pl_funcs.c
--- 905,913 ----
                     SubTransactionId parentSubid, void *arg);
  extern Oid exec_get_datum_type(PLpgSQL_execstate *estate,
                      PLpgSQL_datum *datum);
! extern void exec_get_datum_type_info(PLpgSQL_execstate *estate,
!                          PLpgSQL_datum *datum,
!                          Oid *typeid, int32 *typmod, Oid *collation);

  /* ----------
   * Functions for namespace handling in pl_funcs.c

On Thu, May 12, 2011 at 5:55 PM, Tom Lane <tgl@sss.pgh.pa.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