Thread: Fix oversight in pts_error_callback()

Since we will invoke callback functions unconditionally in errfinish(), so
pts_error_callback() should not report "invalid type name" without checking
current error status.

Regards,
Qingqing


Index: src/backend/parser/parse_type.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/parse_type.c,v
retrieving revision 1.76
diff -c -r1.76 parse_type.c
*** src/backend/parser/parse_type.c     1 Aug 2005 20:31:10 -0000       1.76
--- src/backend/parser/parse_type.c     9 Aug 2005 05:53:08 -0000
***************
*** 359,373 ****
  pts_error_callback(void *arg)
  {
        const char *str = (const char *) arg;
!
!       errcontext("invalid type name \"%s\"", str);

        /*
         * Currently we just suppress any syntax error position report,
rather
         * than transforming to an "internal query" error.      It's
unlikely that
         * a type name is complex enough to need positioning.
         */
!       errposition(0);
  }

  /*
--- 359,379 ----
  pts_error_callback(void *arg)
  {
        const char *str = (const char *) arg;
!       int     syntaxerrposition;

        /*
         * Currently we just suppress any syntax error position report,
rather
         * than transforming to an "internal query" error.      It's
unlikely that
         * a type name is complex enough to need positioning.
         */
!       syntaxerrposition = geterrposition();
!       if (syntaxerrposition > 0)
!       {
!               errcontext("invalid type name \"%s\"", str);
!               errposition(0);
!       }
!       else
!               errcontext("parse type string \"%s\"", str);
  }

  /*

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> Since we will invoke callback functions unconditionally in errfinish(), so
> pts_error_callback() should not report "invalid type name" without checking
> current error status.

Please exhibit a case in which you feel this is needed.

            regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> writes
>
> Please exhibit a case in which you feel this is needed.
>

Suppose I want to print a debug info in parseTypeString() like this:

    + elog(DEBUG1, "parse type %s", buf.data);
     raw_parsetree_list = raw_parser(buf.data);

Rebuild the server, psql it:

    test=# set log_min_messages = debug1;
    SET
    test=# select regtypein('int4');
    DEBUG:  parse type SELECT NULL::int4
    CONTEXT:  invalid type name "int4"
     regtypein
    -----------
     integer
    (1 row)

The CONTEXT info is bogus.

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> "Tom Lane" <tgl@sss.pgh.pa.us> writes
>> Please exhibit a case in which you feel this is needed.

> Suppose I want to print a debug info in parseTypeString() like this:
>     + elog(DEBUG1, "parse type %s", buf.data);
>      raw_parsetree_list = raw_parser(buf.data);

That's a contrived example (and if I believed it, I would think that the
right answer is to emit no errcontext if the elevel is less than ERROR).
Give me an actual use case in which the patch gives a better rather than
worse error report.  I think for most people it would just obfuscate the
message.

            regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> writes:
>
> That's a contrived example (and if I believed it, I would think that the
> right answer is to emit no errcontext if the elevel is less than ERROR).
>

Yes, I've thought about ignore errcontext by considering elevel. But I
scratched the source code for other uses of errcontext, and I found seems
all of them understand that errcontext will be called unconditionally. For
example, buffer_write_error_callback(), it doesn't say "error in writing
block ..." but says "writing  block ...". So I think this place is not
consistent with others - it just says "invalid ... ", and should be changed.


Regards,
Qingqing