Thread: Increased error verbosity when querying row-returning functions

This patch to src/backend/executor/nodeFunctionscan.c is intended to
make life a little easier for people using row-returning functions, by
increasing the level of detail in the error messages thrown when
tupledesc_match fails.

Cheers

BJ


Index: src/backend/executor/nodeFunctionscan.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/executor/nodeFunctionscan.c,v
retrieving revision 1.29
diff -c -r1.29 nodeFunctionscan.c
*** src/backend/executor/nodeFunctionscan.c    31 Dec 2004 21:59:45 -0000    1.29
--- src/backend/executor/nodeFunctionscan.c    11 Jan 2005 22:17:16 -0000
***************
*** 87,96 ****
           * need to do this for functions returning RECORD, but might as
           * well do it always.
           */
!         if (funcTupdesc && !tupledesc_match(node->tupdesc, funcTupdesc))
!             ereport(ERROR,
!                     (errcode(ERRCODE_DATATYPE_MISMATCH),
!                      errmsg("query-specified return row and actual function return row do not match")));
      }

      /*
--- 87,94 ----
           * need to do this for functions returning RECORD, but might as
           * well do it always.
           */
!         if( funcTupdesc )
!             tupledesc_match(node->tupdesc, funcTupdesc);
      }

      /*
***************
*** 363,369 ****
--- 361,373 ----
      int            i;

      if (dst_tupdesc->natts != src_tupdesc->natts)
+     {
+         ereport(ERROR,
+             (errcode(ERRCODE_DATATYPE_MISMATCH),
+              errmsg("function return row and query-specified return row do not match"),
+              errdetail("function-returned row contains %d attributes, but query expects %d.", src_tupdesc->natts,
dst_tupdesc->natts)));
          return false;
+     }

      for (i = 0; i < dst_tupdesc->natts; i++)
      {
***************
*** 373,382 ****
--- 377,401 ----
          if (dattr->atttypid == sattr->atttypid)
              continue;            /* no worries */
          if (!dattr->attisdropped)
+         {
+             ereport(ERROR,
+                 (errcode(ERRCODE_DATATYPE_MISMATCH),
+                  errmsg("function return row and query-specified return row do not match"),
+                  errdetail("function returned type %s at ordinal position %d, but query expects %s.",
+                      format_type_be(sattr->atttypid),
+                      i+1,
+                      format_type_be(dattr->atttypid))));
              return false;
+         }
          if (dattr->attlen != sattr->attlen ||
              dattr->attalign != sattr->attalign)
+         {
+             ereport(ERROR,
+                 (errcode(ERRCODE_DATATYPE_MISMATCH),
+                  errmsg("function return row and query-specified return row do not match"),
+                  errdetail("physical storage mismatch on dropped attribute at ordinal position %d.", i+1)));
              return false;
+         }
      }

      return true;

On Wed, Jan 12, 2005 at 09:23:26AM +1100, Brendan Jurd wrote:
> This patch to src/backend/executor/nodeFunctionscan.c is intended to
> make life a little easier for people using row-returning functions, by
> increasing the level of detail in the error messages thrown when
> tupledesc_match fails.

You should get rid of the returns, because ereport(ERROR) will never
return control to the function and they are thus dead code.  And make
the function return void rather than bool.

Also follow the style: use "if (foo)" rather than "if( foo )".  And
message style stipulates that the errdetail() message should start with
a capital (upper case?) letter.

--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)

Alvaro Herrera wrote:

>On Wed, Jan 12, 2005 at 09:23:26AM +1100, Brendan Jurd wrote:
>
>
>>This patch to src/backend/executor/nodeFunctionscan.c is intended to
>>make life a little easier for people using row-returning functions, by
>>increasing the level of detail in the error messages thrown when
>>tupledesc_match fails.
>>
>>
>
>You should get rid of the returns, because ereport(ERROR) will never
>return control to the function and they are thus dead code.  And make
>the function return void rather than bool.
>
>Also follow the style: use "if (foo)" rather than "if( foo )".  And
>message style stipulates that the errdetail() message should start with
>a capital (upper case?) letter.
>
>
>
Thanks Alvaro, changes made and new patch attached.
Index: src/backend/executor/nodeFunctionscan.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/executor/nodeFunctionscan.c,v
retrieving revision 1.29
diff -c -r1.29 nodeFunctionscan.c
*** src/backend/executor/nodeFunctionscan.c    31 Dec 2004 21:59:45 -0000    1.29
--- src/backend/executor/nodeFunctionscan.c    12 Jan 2005 06:36:53 -0000
***************
*** 36,42 ****


  static TupleTableSlot *FunctionNext(FunctionScanState *node);
! static bool tupledesc_match(TupleDesc dst_tupdesc, TupleDesc src_tupdesc);

  /* ----------------------------------------------------------------
   *                        Scan Support
--- 36,42 ----


  static TupleTableSlot *FunctionNext(FunctionScanState *node);
! static void tupledesc_match(TupleDesc dst_tupdesc, TupleDesc src_tupdesc);

  /* ----------------------------------------------------------------
   *                        Scan Support
***************
*** 87,96 ****
           * need to do this for functions returning RECORD, but might as
           * well do it always.
           */
!         if (funcTupdesc && !tupledesc_match(node->tupdesc, funcTupdesc))
!             ereport(ERROR,
!                     (errcode(ERRCODE_DATATYPE_MISMATCH),
!                      errmsg("query-specified return row and actual function return row do not match")));
      }

      /*
--- 87,94 ----
           * need to do this for functions returning RECORD, but might as
           * well do it always.
           */
!         if (funcTupdesc)
!             tupledesc_match(node->tupdesc, funcTupdesc);
      }

      /*
***************
*** 357,369 ****
   * destination type, so long as the physical storage matches.  This is
   * helpful in some cases involving out-of-date cached plans.
   */
! static bool
  tupledesc_match(TupleDesc dst_tupdesc, TupleDesc src_tupdesc)
  {
      int            i;

      if (dst_tupdesc->natts != src_tupdesc->natts)
!         return false;

      for (i = 0; i < dst_tupdesc->natts; i++)
      {
--- 355,370 ----
   * destination type, so long as the physical storage matches.  This is
   * helpful in some cases involving out-of-date cached plans.
   */
! static void
  tupledesc_match(TupleDesc dst_tupdesc, TupleDesc src_tupdesc)
  {
      int            i;

      if (dst_tupdesc->natts != src_tupdesc->natts)
!         ereport(ERROR,
!             (errcode(ERRCODE_DATATYPE_MISMATCH),
!              errmsg("function return row and query-specified return row do not match"),
!              errdetail("Function-returned row contains %d attributes, but query expects %d.", src_tupdesc->natts,
dst_tupdesc->natts)));

      for (i = 0; i < dst_tupdesc->natts; i++)
      {
***************
*** 373,382 ****
          if (dattr->atttypid == sattr->atttypid)
              continue;            /* no worries */
          if (!dattr->attisdropped)
!             return false;
          if (dattr->attlen != sattr->attlen ||
              dattr->attalign != sattr->attalign)
!             return false;
      }

      return true;
--- 374,393 ----
          if (dattr->atttypid == sattr->atttypid)
              continue;            /* no worries */
          if (!dattr->attisdropped)
!             ereport(ERROR,
!                 (errcode(ERRCODE_DATATYPE_MISMATCH),
!                  errmsg("function return row and query-specified return row do not match"),
!                  errdetail("Function returned type %s at ordinal position %d, but query expects %s.",
!                      format_type_be(sattr->atttypid),
!                      i+1,
!                      format_type_be(dattr->atttypid))));
!
          if (dattr->attlen != sattr->attlen ||
              dattr->attalign != sattr->attalign)
!             ereport(ERROR,
!                 (errcode(ERRCODE_DATATYPE_MISMATCH),
!                  errmsg("function return row and query-specified return row do not match"),
!                  errdetail("Physical storage mismatch on dropped attribute at ordinal position %d.", i+1)));
      }

      return true;

Brendan Jurd wrote:

> Alvaro Herrera wrote:
>
>> On Wed, Jan 12, 2005 at 09:23:26AM +1100, Brendan Jurd wrote:
>>
>>
>>> This patch to src/backend/executor/nodeFunctionscan.c is intended to
>>> make life a little easier for people using row-returning functions,
>>> by increasing the level of detail in the error messages thrown when
>>> tupledesc_match fails.
>>>
>>
>>
>> You should get rid of the returns, because ereport(ERROR) will never
>> return control to the function and they are thus dead code.  And make
>> the function return void rather than bool.
>>
>> Also follow the style: use "if (foo)" rather than "if( foo )".  And
>> message style stipulates that the errdetail() message should start with
>> a capital (upper case?) letter.
>>
>>
>>
> Thanks Alvaro, changes made and new patch attached.
>
>
>

I submitted this patch about 5 days ago and I haven't heard anything
since.  I don't wish to be rude, but I'm not familiar with the
pgsql-patches etiquette yet, and I noticed most submissions and
questions are getting responses very quickly.  5 days' silence seems
outside the norm for this list.  I'm just looking for some reassurance
that the patch hasn't "fallen off the grid".

Thanks

BJ

Brendan Jurd <blakjak@blakjak.sytes.net> writes:
>> Thanks Alvaro, changes made and new patch attached.

> I submitted this patch about 5 days ago and I haven't heard anything
> since.  I don't wish to be rude, but I'm not familiar with the
> pgsql-patches etiquette yet, and I noticed most submissions and
> questions are getting responses very quickly.

This is in the category of "stuff that has to wait for 8.1", so nothing
will be done with it until after we fork the CVS tree (which should
happen any day now).  If we weren't busy with getting 8.0 out the door,
there'd probably be more response, but right now small patches are just
going into the to-look-at-later folder...

            regards, tom lane

Tom Lane wrote:

>Brendan Jurd <blakjak@blakjak.sytes.net> writes:
>
>
>>>Thanks Alvaro, changes made and new patch attached.
>>>
>>>
>
>
>
>>I submitted this patch about 5 days ago and I haven't heard anything
>>since.  I don't wish to be rude, but I'm not familiar with the
>>pgsql-patches etiquette yet, and I noticed most submissions and
>>questions are getting responses very quickly.
>>
>>
>
>This is in the category of "stuff that has to wait for 8.1", so nothing
>will be done with it until after we fork the CVS tree (which should
>happen any day now).  If we weren't busy with getting 8.0 out the door,
>there'd probably be more response, but right now small patches are just
>going into the to-look-at-later folder...
>
>            regards, tom lane
>
>
That's cool, I've got nothing against it being in the to-look-at-later
folder.  Just glad it didn't end up in /dev/null by mistake =)

Thanks for the clarification.

BJ

This has been saved for the 8.1 release:

    http:/momjian.postgresql.org/cgi-bin/pgpatches2

---------------------------------------------------------------------------

Brendan Jurd wrote:
> Alvaro Herrera wrote:
>
> >On Wed, Jan 12, 2005 at 09:23:26AM +1100, Brendan Jurd wrote:
> >
> >
> >>This patch to src/backend/executor/nodeFunctionscan.c is intended to
> >>make life a little easier for people using row-returning functions, by
> >>increasing the level of detail in the error messages thrown when
> >>tupledesc_match fails.
> >>
> >>
> >
> >You should get rid of the returns, because ereport(ERROR) will never
> >return control to the function and they are thus dead code.  And make
> >the function return void rather than bool.
> >
> >Also follow the style: use "if (foo)" rather than "if( foo )".  And
> >message style stipulates that the errdetail() message should start with
> >a capital (upper case?) letter.
> >
> >
> >
> Thanks Alvaro, changes made and new patch attached.

> Index: src/backend/executor/nodeFunctionscan.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/executor/nodeFunctionscan.c,v
> retrieving revision 1.29
> diff -c -r1.29 nodeFunctionscan.c
> *** src/backend/executor/nodeFunctionscan.c    31 Dec 2004 21:59:45 -0000    1.29
> --- src/backend/executor/nodeFunctionscan.c    12 Jan 2005 06:36:53 -0000
> ***************
> *** 36,42 ****
>
>
>   static TupleTableSlot *FunctionNext(FunctionScanState *node);
> ! static bool tupledesc_match(TupleDesc dst_tupdesc, TupleDesc src_tupdesc);
>
>   /* ----------------------------------------------------------------
>    *                        Scan Support
> --- 36,42 ----
>
>
>   static TupleTableSlot *FunctionNext(FunctionScanState *node);
> ! static void tupledesc_match(TupleDesc dst_tupdesc, TupleDesc src_tupdesc);
>
>   /* ----------------------------------------------------------------
>    *                        Scan Support
> ***************
> *** 87,96 ****
>            * need to do this for functions returning RECORD, but might as
>            * well do it always.
>            */
> !         if (funcTupdesc && !tupledesc_match(node->tupdesc, funcTupdesc))
> !             ereport(ERROR,
> !                     (errcode(ERRCODE_DATATYPE_MISMATCH),
> !                      errmsg("query-specified return row and actual function return row do not match")));
>       }
>
>       /*
> --- 87,94 ----
>            * need to do this for functions returning RECORD, but might as
>            * well do it always.
>            */
> !         if (funcTupdesc)
> !             tupledesc_match(node->tupdesc, funcTupdesc);
>       }
>
>       /*
> ***************
> *** 357,369 ****
>    * destination type, so long as the physical storage matches.  This is
>    * helpful in some cases involving out-of-date cached plans.
>    */
> ! static bool
>   tupledesc_match(TupleDesc dst_tupdesc, TupleDesc src_tupdesc)
>   {
>       int            i;
>
>       if (dst_tupdesc->natts != src_tupdesc->natts)
> !         return false;
>
>       for (i = 0; i < dst_tupdesc->natts; i++)
>       {
> --- 355,370 ----
>    * destination type, so long as the physical storage matches.  This is
>    * helpful in some cases involving out-of-date cached plans.
>    */
> ! static void
>   tupledesc_match(TupleDesc dst_tupdesc, TupleDesc src_tupdesc)
>   {
>       int            i;
>
>       if (dst_tupdesc->natts != src_tupdesc->natts)
> !         ereport(ERROR,
> !             (errcode(ERRCODE_DATATYPE_MISMATCH),
> !              errmsg("function return row and query-specified return row do not match"),
> !              errdetail("Function-returned row contains %d attributes, but query expects %d.", src_tupdesc->natts,
dst_tupdesc->natts)));
>
>       for (i = 0; i < dst_tupdesc->natts; i++)
>       {
> ***************
> *** 373,382 ****
>           if (dattr->atttypid == sattr->atttypid)
>               continue;            /* no worries */
>           if (!dattr->attisdropped)
> !             return false;
>           if (dattr->attlen != sattr->attlen ||
>               dattr->attalign != sattr->attalign)
> !             return false;
>       }
>
>       return true;
> --- 374,393 ----
>           if (dattr->atttypid == sattr->atttypid)
>               continue;            /* no worries */
>           if (!dattr->attisdropped)
> !             ereport(ERROR,
> !                 (errcode(ERRCODE_DATATYPE_MISMATCH),
> !                  errmsg("function return row and query-specified return row do not match"),
> !                  errdetail("Function returned type %s at ordinal position %d, but query expects %s.",
> !                      format_type_be(sattr->atttypid),
> !                      i+1,
> !                      format_type_be(dattr->atttypid))));
> !
>           if (dattr->attlen != sattr->attlen ||
>               dattr->attalign != sattr->attalign)
> !             ereport(ERROR,
> !                 (errcode(ERRCODE_DATATYPE_MISMATCH),
> !                  errmsg("function return row and query-specified return row do not match"),
> !                  errdetail("Physical storage mismatch on dropped attribute at ordinal position %d.", i+1)));
>       }
>
>       return true;

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
>                http://www.postgresql.org/docs/faqs/FAQ.html

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Wed, 2005-01-12 at 17:34 +1100, Brendan Jurd wrote:
> Thanks Alvaro, changes made and new patch attached.

Applied to HEAD -- thanks for the patch.

Another style tip: we don't accept code that produces compiler warnings
(pretty much), so checking that before submitting patches would be nice.

-Neil