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

On Fri, 05 Sep 2008, Tom Lane <tgl@sss.pgh.pa.us> writes:
> I think the best way is to use
>
>     subroutine(..., gettext_noop("special error message here"))
>
> at the call sites, and then
>
>     errmsg("%s", _(msg))
>
> when throwing the error.  gettext_noop() is needed to have the string
> be put into the message catalog, but it doesn't represent any run-time
> effort.  The _() macro is what actually makes the translation lookup
> happen.  We don't want to incur that cost in the normal no-error case,
> which is why you shouldn't just do _() at the call site and pass an
> already-translated string to the subroutine.

Modified as you suggested. BTW, will there be a similar i18n scenario
for "dropped column" you mentioned below?

>>   if (td1->attrs[i]->atttypid &&
>>       td2->attrs[i]->atttypid &&
>>       td1->attrs[i]->atttypid != td2->attrs[i]->atttypid)
>
>> expression to fix this?
>
> No, that's weakening the compatibility check.  (There's a separate issue
> here of teaching plpgsql to actually cope nicely with rowtypes
> containing dropped columns, but that's well beyond the scope of this
> patch.)
>
> What I'm on about is protecting format_type_be() from being passed
> a zero and then failing.  Perhaps it would be good enough to do
> something like
>
>     OidIsValid(td1->attrs[i]->atttypid) ?
>            format_type_with_typemod(td1->attrs[i]->atttypid,
>                         td1->attrs[i]->atttypmod) :
>            "dropped column"
>
> while throwing the error.
>
> BTW, since what's actually being looked at is just the type OID and not
> the typmod, it seems inappropriate to me to show the typmod in the
> error.  I'd go with just format_type_be(td1->attrs[i]->atttypid) here
> I think.

Done with format_type_be() usage.

BTW, Alvaro fixed my string concatenations which yielded in lines
exceeding 80 characters width, but I'd want to ask twice if you're sure
with this. Because, IMHO, PostgreSQL is also famous with the quality and
readability of its source code -- that I'm quite proud of as a user,
kudos to developers -- and I think it'd be better to stick with 80
characters width convention as much as one can.


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.219
diff -c -r1.219 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c    1 Sep 2008 22:30:33 -0000    1.219
--- src/pl/plpgsql/src/pl_exec.c    5 Sep 2008 18:19:50 -0000
***************
*** 188,194 ****
                         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);
--- 188,195 ----
                         Oid reqtype, int32 reqtypmod,
                         bool isnull);
  static void exec_init_tuple_store(PLpgSQL_execstate *estate);
! static void validate_tupdesc_compat(TupleDesc expected, TupleDesc returned,
!                                     const 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);
***************
*** 384,394 ****
              {
                  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:

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

***************
*** 705,715 ****
          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));
      }
--- 703,711 ----
          rettup = NULL;
      else
      {
!         validate_tupdesc_compat(trigdata->tg_relation->rd_att,
!                                 estate.rettupdesc,
!                                 gettext_noop("returned tuple structure does not match table of trigger event"));
          /* Copy tuple to upper executor memory */
          rettup = SPI_copytuple((HeapTuple) DatumGetPointer(estate.retval));
      }
***************
*** 2199,2209 ****
                            (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;
--- 2195,2204 ----
                            (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(tupdesc, rec->tupdesc,
!                                             gettext_noop("wrong record type supplied in RETURN NEXT"));
                      tuple = rec->tup;
                  }
                  break;
***************
*** 2309,2318 ****
                                             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)
      {
--- 2304,2311 ----
                                             stmt->params);
      }

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

      while (true)
      {
***************
*** 5145,5167 ****
  }

  /*
!  * 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;
  }

  /* ----------
--- 5138,5176 ----
  }

  /*
!  * Validates compatibility of supplied TupleDesc pair by checking number and type
!  * of attributes.
   */
! static void
! validate_tupdesc_compat(TupleDesc expected, TupleDesc returned, const char *msg)
  {
!     int        i;

!     if (!expected || !returned)
!         ereport(ERROR,
!                 (errcode(ERRCODE_DATATYPE_MISMATCH),
!                  errmsg("%s", _(msg))));

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

!     for (i = 0; i < expected->natts; i++)
!         if (expected->attrs[i]->atttypid != returned->attrs[i]->atttypid)
!             ereport(ERROR,
!                     (errcode(ERRCODE_DATATYPE_MISMATCH),
!                      errmsg("%s", _(msg)),
!                      errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
!                                OidIsValid(returned->attrs[i]->atttypid)
!                                ? format_type_be(returned->attrs[i]->atttypid)
!                                : "dropped column",
!                                OidIsValid(expected->attrs[i]->atttypid)
!                                ? format_type_be(expected->attrs[i]->atttypid)
!                                : "dropped column",
!                                NameStr(expected->attrs[i]->attname))));
  }

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