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

On Mon, 8 Sep 2008, Alvaro Herrera <alvherre@commandprompt.com> writes:
>> Modified as you suggested. BTW, will there be a similar i18n scenario
>> for "dropped column" you mentioned below?
>
> Yes, you need _() around those too.

For this purpose, I introduced a dropped_column_type variable in
validate_tupdesc_compat() function:

  const char dropped_column_type[] = _("n/a (dropped column)");

And used this in below fashion:

  OidIsValid(returned->attrs[i]->atttypid)
  ? format_type_be(returned->attrs[i]->atttypid)
  : dropped_column_type

>> Done with format_type_be() usage.
>
> BTW you forgot to remove the quotes around the type names (I know I told
> you to add them but Tom gave the reasons why it's not needed) :-)

Done.

> Those are minor problems that are easily fixed.  However there's a
> larger issue that Tom mentioned earlier and I concur, which is that the
> caller is forming the primary error message and passing it down.  It
> gets a bit silly if you consider the ways the messages end up worded:
>
>    errmsg("returned record type does not match expected record type"));
>    errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
>    ---> this is the case where it's OK
>
>    errmsg("wrong record type supplied in RETURN NEXT"));
>    errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
>    --> this is strange
>
>    errmsg("returned tuple structure does not match table of trigger event"));
>    errdetail("Returned type \"%s\" does not match expected type \"%s\" in column \"%s\".",
>    --> this is not OK

What we're trying to do is to avoid code duplication while checking the
returned tuple types against expected tuple types. For this purpose we
implemented a generic function (validate_tupdesc_compat) to handle all
possible cases. But, IMHO, it's important to remember that there is no
perfect generic function to satisfy all possible cases at best.

> I've been thinking how to pass down the context information without
> feeding the complete string, but I don't find a way without doing
> message construction. Construction is to be avoided because it's a
> pain for translators.
>
> Maybe we should just use something generic like errmsg("mismatching
> record type") and have the caller pass two strings specifying what's
> the "returned" tuple and what's the "expected", but I don't see how
> ...  (BTW this is worth fixing, because every case seems to have
> appeared independently without much thought as to other callsites with
> the same pattern.)

I considered the subject with identical constraints but couldn't come up
with a more rational solution. Nevertheless, I'm open to any suggestion.


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    9 Sep 2008 05:48:57 -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,5177 ----
  }

  /*
!  * 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;
!     const char dropped_column_type[] = _("n/a (dropped column)");

!     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_type,
!                                OidIsValid(expected->attrs[i]->atttypid)
!                                ? format_type_be(expected->attrs[i]->atttypid)
!                                : dropped_column_type,
!                                NameStr(expected->attrs[i]->attname))));
  }

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