Re: Verbosity of Function Return Type Checks - Mailing list pgsql-hackers

[Please ignore the previous reply.]

On Fri, 8 Aug 2008, Alvaro Herrera <alvherre@commandprompt.com> writes:
> I think this is a good idea, but the new error messages need more work.
> Have a look at the message style guidelines please,
> http://www.postgresql.org/docs/8.3/static/error-style-guide.html

Right. Done -- I hope.

> Particularly I think you need to keep the original errmsg() and add the
> new messages as errdetail().

Made callers pass related error message as a string parameter, and
appended required details using errdetail().

> (I notice that there's the slight problem
> that the error messages are different for the different callers.)

Above mentioned change should have addressed this issue too.

> Also, please use context diffs.

Done.


Regards.

Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.216
diff -c -r1.216 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c    16 May 2008 18:34:51 -0000    1.216
--- src/pl/plpgsql/src/pl_exec.c    9 Aug 2008 10:10:32 -0000
***************
*** 190,196 ****
                         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);
  static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
  static void free_var(PLpgSQL_var *var);
--- 190,196 ----
                         Oid reqtype, int32 reqtypmod,
                         bool isnull);
  static void exec_init_tuple_store(PLpgSQL_execstate *estate);
! static void validate_tupdesc_compat(TupleDesc td1, TupleDesc td2, char *msg);
  static void exec_set_found(PLpgSQL_execstate *estate, bool state);
  static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
  static void free_var(PLpgSQL_var *var);
***************
*** 386,396 ****
              {
                  case TYPEFUNC_COMPOSITE:
                      /* got the expected result rowtype, now check it */
!                     if (estate.rettupdesc == NULL ||
!                         !compatible_tupdesc(estate.rettupdesc, tupdesc))
!                         ereport(ERROR,
!                                 (errcode(ERRCODE_DATATYPE_MISMATCH),
!                                  errmsg("returned record type does not match expected record type")));
                      break;
                  case TYPEFUNC_RECORD:

--- 386,394 ----
              {
                  case TYPEFUNC_COMPOSITE:
                      /* got the expected result rowtype, now check it */
!                     validate_tupdesc_compat(tupdesc, estate.rettupdesc,
!                                             "returned record type does not "
!                                             "match expected record type");
                      break;
                  case TYPEFUNC_RECORD:

***************
*** 707,717 ****
          rettup = NULL;
      else
      {
!         if (!compatible_tupdesc(estate.rettupdesc,
!                                 trigdata->tg_relation->rd_att))
!             ereport(ERROR,
!                     (errcode(ERRCODE_DATATYPE_MISMATCH),
!                      errmsg("returned tuple structure does not match table of trigger event")));
          /* Copy tuple to upper executor memory */
          rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
      }
--- 705,714 ----
          rettup = NULL;
      else
      {
!         validate_tupdesc_compat(trigdata->tg_relation->rd_att,
!                                 estate.rettupdesc,
!                                 "returned tuple structure does not match "
!                                 "table of trigger event");
          /* Copy tuple to upper executor memory */
          rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
      }
***************
*** 2201,2211 ****
                            (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.")));
!                     if (!compatible_tupdesc(tupdesc, rec->tupdesc))
!                         ereport(ERROR,
!                                 (errcode(ERRCODE_DATATYPE_MISMATCH),
!                         errmsg("wrong record type supplied in RETURN NEXT")));
                      tuple = rec->tup;
                  }
                  break;
--- 2198,2208 ----
                            (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.")));
!                     validate_tupdesc_compat(rec->tupdesc, tupdesc,
!                                             "wrong record type supplied in "
!                                             "RETURN NEXT");
                      tuple = rec->tup;
                  }
                  break;
***************
*** 2311,2320 ****
                                             stmt->params);
      }

!     if (!compatible_tupdesc(estate->rettupdesc, portal->tupDesc))
!         ereport(ERROR,
!                 (errcode(ERRCODE_DATATYPE_MISMATCH),
!           errmsg("structure of query does not match function result type")));

      while (true)
      {
--- 2308,2316 ----
                                             stmt->params);
      }

!     validate_tupdesc_compat(portal->tupDesc, estate->rettupdesc,
!                             "structure of query does not match function "
!                             "result type");

      while (true)
      {
***************
*** 5138,5160 ****
  }

  /*
!  * Check two tupledescs have matching number and types of attributes
   */
! static bool
! compatible_tupdesc(TupleDesc td1, TupleDesc td2)
  {
!     int            i;

      if (td1->natts != td2->natts)
!         return false;

      for (i = 0; i < td1->natts; i++)
-     {
          if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
!             return false;
!     }
!
!     return true;
  }

  /* ----------
--- 5134,5170 ----
  }

  /*
!  * Validates compatibility of supplied TupleDesc couple by checking # and type
!  * of available arguments.
   */
! static void
! validate_tupdesc_compat(TupleDesc td1, TupleDesc td2, char *msg)
  {
!     int i;
!
!     if (!td1 || !td2)
!         ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg(msg)));

      if (td1->natts != td2->natts)
!         ereport(ERROR,
!                 (errcode(ERRCODE_DATATYPE_MISMATCH),
!                  errmsg(msg),
!                  errdetail("Number of returned columns (%d) does not match "
!                            "expected column count (%d).",
!                            td1->natts, td2->natts)));

      for (i = 0; i < td1->natts; i++)
          if (td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
!             ereport(ERROR,
!                     (errcode(ERRCODE_DATATYPE_MISMATCH),
!                      errmsg(msg),
!                      errdetail("Returned record type (%s) does not match "
!                                "expected record type (%s) in column %d (%s).",
!                                format_type_with_typemod(td1->attrs[i]->atttypid,
!                                                         td1->attrs[i]->atttypmod),
!                                format_type_with_typemod(td2->attrs[i]->atttypid,
!                                                         td2->attrs[i]->atttypmod),
!                                (1+i), NameStr(td2->attrs[i]->attname))));
  }

  /* ----------