Thread: PLPGSQL OID Bug

PLPGSQL OID Bug

From
"Kevin McArthur"
Date:
This patch will resolve the oid retrieval bugs from plpgsql. There are however several other places where isnull=false was removed and replaced with isnull which may also need to be corrected.
 
Kevin McArthur
StormTide Digital Studios Inc.
 
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.149
diff -c -r1.149 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c        26 Jun 2005 22:05:42 -0000      1.149
--- src/pl/plpgsql/src/pl_exec.c        27 Jul 2005 20:38:25 -0000
***************
*** 1143,1149 ****
        {
                PLpgSQL_diag_item       *diag_item = (PLpgSQL_diag_item *) lfirst(lc);
                PLpgSQL_datum           *var;
!               bool                             isnull;
 
                if (diag_item->target <= 0)
                        continue;
--- 1143,1149 ----
        {
                PLpgSQL_diag_item       *diag_item = (PLpgSQL_diag_item *) lfirst(lc);
                PLpgSQL_datum           *var;
!               bool                             isnull=false;
 
                if (diag_item->target <= 0)
                        continue;
Attachment

Re: PLPGSQL OID Bug

From
Tom Lane
Date:
"Kevin McArthur" <Kevin@StormTide.ca> writes:
> This patch will resolve the oid retrieval bugs from plpgsql.

Applied.

> There are
> however several other places where isnull=false was removed and
> replaced with isnull which may also need to be corrected.

The other spots seem to be OK.  Thanks for the report and fix!

            regards, tom lane

Re: PLPGSQL OID Bug

From
Neil Conway
Date:
Tom Lane wrote:
> The other spots seem to be OK.  Thanks for the report and fix!

Woops, my apologies for introducing the bug. In general I think it's
worth removing explicit initialization of out parameters unless that
initialization is actually needed. I thought I had checked that
`isnull=false' wasn't needed, but obviously I missed a case.

BTW, is there a reason why exec_cast_value() and friends take a bool *,
rather than a bool?

-Neil

Re: PLPGSQL OID Bug

From
Tom Lane
Date:
Neil Conway <neilc@samurai.com> writes:
> BTW, is there a reason why exec_cast_value() and friends take a bool *,
> rather than a bool?

I think the reason is that it's both an input and an output parameter,
to handle the case where the cast function returns NULL.

This is obviously a pretty bug-prone convention, however, and so maybe
we should rethink it.

            regards, tom lane

Re: PLPGSQL OID Bug

From
Neil Conway
Date:
Tom Lane wrote:
> I think the reason is that it's both an input and an output parameter,
> to handle the case where the cast function returns NULL.

The only reference to `isnull' in the body of exec_cast_value() is:

if (!*isnull)
{
     /* ... */
}

i.e. it is never referenced again, let alone written through. Barring
any objections I'll apply the attached patch tomorrow.

-Neil
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /var/lib/cvs/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.150
diff -c -r1.150 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c    28 Jul 2005 00:26:30 -0000    1.150
--- src/pl/plpgsql/src/pl_exec.c    28 Jul 2005 05:29:21 -0000
***************
*** 173,182 ****
                  FmgrInfo *reqinput,
                  Oid reqtypioparam,
                  int32 reqtypmod,
!                 bool *isnull);
  static Datum exec_simple_cast_value(Datum value, Oid valtype,
                         Oid reqtype, int32 reqtypmod,
!                        bool *isnull);
  static void exec_init_tuple_store(PLpgSQL_execstate *estate);
  static bool compatible_tupdesc(TupleDesc td1, TupleDesc td2);
  static void exec_set_found(PLpgSQL_execstate *estate, bool state);
--- 173,182 ----
                  FmgrInfo *reqinput,
                  Oid reqtypioparam,
                  int32 reqtypmod,
!                 bool isnull);
  static Datum exec_simple_cast_value(Datum value, Oid valtype,
                         Oid reqtype, int32 reqtypmod,
!                        bool isnull);
  static void exec_init_tuple_store(PLpgSQL_execstate *estate);
  static bool compatible_tupdesc(TupleDesc td1, TupleDesc td2);
  static void exec_set_found(PLpgSQL_execstate *estate, bool state);
***************
*** 356,362 ****
                                              &(func->fn_retinput),
                                              func->fn_rettypioparam,
                                              -1,
!                                             &fcinfo->isnull);

              /*
               * If the function's return type isn't by value, copy the value
--- 356,362 ----
                                              &(func->fn_retinput),
                                              func->fn_rettypioparam,
                                              -1,
!                                             fcinfo->isnull);

              /*
               * If the function's return type isn't by value, copy the value
***************
*** 1348,1354 ****
      value = exec_cast_value(value, valtype, var->datatype->typoid,
                              &(var->datatype->typinput),
                              var->datatype->typioparam,
!                             var->datatype->atttypmod, &isnull);
      if (isnull)
          ereport(ERROR,
                  (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
--- 1348,1354 ----
      value = exec_cast_value(value, valtype, var->datatype->typoid,
                              &(var->datatype->typinput),
                              var->datatype->typioparam,
!                             var->datatype->atttypmod, isnull);
      if (isnull)
          ereport(ERROR,
                  (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
***************
*** 1364,1370 ****
      value = exec_cast_value(value, valtype, var->datatype->typoid,
                              &(var->datatype->typinput),
                              var->datatype->typioparam,
!                             var->datatype->atttypmod, &isnull);
      if (isnull)
          ereport(ERROR,
                  (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
--- 1364,1370 ----
      value = exec_cast_value(value, valtype, var->datatype->typoid,
                              &(var->datatype->typinput),
                              var->datatype->typioparam,
!                             var->datatype->atttypmod, isnull);
      if (isnull)
          ereport(ERROR,
                  (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
***************
*** 1868,1874 ****
                                                  var->datatype->typoid,
                                                  tupdesc->attrs[0]->atttypid,
                                                  tupdesc->attrs[0]->atttypmod,
!                                                 &isNull);

                  tuple = heap_form_tuple(tupdesc, &retval, &isNull);

--- 1868,1874 ----
                                                  var->datatype->typoid,
                                                  tupdesc->attrs[0]->atttypid,
                                                  tupdesc->attrs[0]->atttypmod,
!                                                 isNull);

                  tuple = heap_form_tuple(tupdesc, &retval, &isNull);

***************
*** 1934,1940 ****
                                          rettype,
                                          tupdesc->attrs[0]->atttypid,
                                          tupdesc->attrs[0]->atttypmod,
!                                         &isNull);

          tuple = heap_form_tuple(tupdesc, &retval, &isNull);

--- 1934,1940 ----
                                          rettype,
                                          tupdesc->attrs[0]->atttypid,
                                          tupdesc->attrs[0]->atttypmod,
!                                         isNull);

          tuple = heap_form_tuple(tupdesc, &retval, &isNull);

***************
*** 2995,3001 ****
                                             &(var->datatype->typinput),
                                             var->datatype->typioparam,
                                             var->datatype->atttypmod,
!                                            isNull);

                  if (*isNull && var->notnull)
                      ereport(ERROR,
--- 2995,3001 ----
                                             &(var->datatype->typinput),
                                             var->datatype->typioparam,
                                             var->datatype->atttypmod,
!                                            *isNull);

                  if (*isNull && var->notnull)
                      ereport(ERROR,
***************
*** 3194,3200 ****
                                                       valtype,
                                                       atttype,
                                                       atttypmod,
!                                                      &attisnull);
                  if (attisnull)
                      nulls[fno] = 'n';
                  else
--- 3194,3200 ----
                                                       valtype,
                                                       atttype,
                                                       atttypmod,
!                                                      attisnull);
                  if (attisnull)
                      nulls[fno] = 'n';
                  else
***************
*** 3340,3346 ****
                                                         valtype,
                                                         arrayelemtypeid,
                                                         -1,
!                                                        isNull);

                  /*
                   * Build the modified array value.
--- 3340,3346 ----
                                                         valtype,
                                                         arrayelemtypeid,
                                                         -1,
!                                                        *isNull);

                  /*
                   * Build the modified array value.
***************
*** 3564,3570 ****
      exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
      exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
                                         INT4OID, -1,
!                                        isNull);
      return DatumGetInt32(exprdatum);
  }

--- 3564,3570 ----
      exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
      exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
                                         INT4OID, -1,
!                                        *isNull);
      return DatumGetInt32(exprdatum);
  }

***************
*** 3586,3592 ****
      exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
      exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
                                         BOOLOID, -1,
!                                        isNull);
      return DatumGetBool(exprdatum);
  }

--- 3586,3592 ----
      exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
      exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
                                         BOOLOID, -1,
!                                        *isNull);
      return DatumGetBool(exprdatum);
  }

***************
*** 4060,4068 ****
                  FmgrInfo *reqinput,
                  Oid reqtypioparam,
                  int32 reqtypmod,
!                 bool *isnull)
  {
!     if (!*isnull)
      {
          /*
           * If the type of the queries return value isn't that of the
--- 4060,4068 ----
                  FmgrInfo *reqinput,
                  Oid reqtypioparam,
                  int32 reqtypmod,
!                 bool isnull)
  {
!     if (!isnull)
      {
          /*
           * If the type of the queries return value isn't that of the
***************
*** 4095,4103 ****
  static Datum
  exec_simple_cast_value(Datum value, Oid valtype,
                         Oid reqtype, int32 reqtypmod,
!                        bool *isnull)
  {
!     if (!*isnull)
      {
          if (valtype != reqtype || reqtypmod != -1)
          {
--- 4095,4103 ----
  static Datum
  exec_simple_cast_value(Datum value, Oid valtype,
                         Oid reqtype, int32 reqtypmod,
!                        bool isnull)
  {
!     if (!isnull)
      {
          if (valtype != reqtype || reqtypmod != -1)
          {

Re: PLPGSQL OID Bug

From
Tom Lane
Date:
Neil Conway <neilc@samurai.com> writes:
> Tom Lane wrote:
>> I think the reason is that it's both an input and an output parameter,
>> to handle the case where the cast function returns NULL.

> [ no it ain't ]

In that case feel free to clean it up ...

            regards, tom lane