Re: syntax error position "CREATE FUNCTION" bug fix - Mailing list pgsql-patches

Attached is a proposed patch that fixes the
cursor-position-in-CREATE-FUNCTION issue per my earlier suggestion.

Since I complained that your proposal was too grotty, it's only fair to
throw this out to let people take potshots at it ;-).  The main
grottiness here is providing access to the executing Portal so that the
callback function can get at the original command string.  I don't think
this is unreasonably bad, since we already have a global PortalContext
that points to the portal's memory context --- I just added parallel
code that updates a new global ActivePortal.

The re-parsing of the original command is simplistic but will handle all
normal cases.

            regards, tom lane


*** src/backend/catalog/pg_proc.c.orig    Sat Mar 13 20:58:41 2004
--- src/backend/catalog/pg_proc.c    Thu Mar 18 18:20:20 2004
***************
*** 23,31 ****
--- 23,33 ----
  #include "executor/executor.h"
  #include "fmgr.h"
  #include "miscadmin.h"
+ #include "mb/pg_wchar.h"
  #include "parser/parse_coerce.h"
  #include "parser/parse_expr.h"
  #include "parser/parse_type.h"
+ #include "tcop/pquery.h"
  #include "tcop/tcopprot.h"
  #include "utils/acl.h"
  #include "utils/builtins.h"
***************
*** 45,50 ****
--- 47,56 ----
  static Datum create_parameternames_array(int parameterCount,
                                           const char *parameterNames[]);
  static void sql_function_parse_error_callback(void *arg);
+ static int    match_prosrc_to_query(const char *prosrc, const char *queryText,
+                                   int cursorpos);
+ static bool match_prosrc_to_literal(const char *prosrc, const char *literal,
+                                     int cursorpos, int *newcursorpos);


  /* ----------------------------------------------------------------
***************
*** 768,774 ****
           * location is inside the function body, not out in CREATE FUNCTION.
           */
          sqlerrcontext.callback = sql_function_parse_error_callback;
!         sqlerrcontext.arg = proc;
          sqlerrcontext.previous = error_context_stack;
          error_context_stack = &sqlerrcontext;

--- 774,780 ----
           * location is inside the function body, not out in CREATE FUNCTION.
           */
          sqlerrcontext.callback = sql_function_parse_error_callback;
!         sqlerrcontext.arg = tuple;
          sqlerrcontext.previous = error_context_stack;
          error_context_stack = &sqlerrcontext;

***************
*** 800,821 ****
  }

  /*
!  * error context callback to let us supply a context marker
   */
  static void
  sql_function_parse_error_callback(void *arg)
  {
!     Form_pg_proc proc = (Form_pg_proc) arg;

      /*
!      * XXX it'd be really nice to adjust the syntax error position to
!      * account for the offset from the start of the statement to the
!      * function body string, not to mention any quoting characters in
!      * the string, but I can't see any decent way to do that...
       *
!      * In the meantime, put in a CONTEXT entry that can cue clients
!      * not to trust the syntax error position completely.
       */
!     errcontext("SQL function \"%s\"",
!                NameStr(proc->proname));
  }
--- 806,976 ----
  }

  /*
!  * error context callback to let us adjust syntax errors from SQL functions
   */
  static void
  sql_function_parse_error_callback(void *arg)
  {
!     HeapTuple    tuple = (HeapTuple) arg;
!     Form_pg_proc proc = (Form_pg_proc) GETSTRUCT(tuple);
!     int            origerrposition;
!     const char *queryText;
!     bool        isnull;
!     Datum        tmp;
!     char       *prosrc;
!     int            newerrposition;
!
!     /*
!      * Nothing to do unless we are dealing with a syntax error that has
!      * a cursor position.  In that case, we need to try to adjust the
!      * position to match the original query, not the text of the function.
!      */
!     origerrposition = geterrposition();
!     if (origerrposition <= 0)
!         return;
!
!     /*
!      * We can get the original query text from the active portal (hack...)
!      */
!     Assert(ActivePortal && ActivePortal->portalActive);
!     queryText = ActivePortal->sourceText;
!
!     /*
!      * Try to locate the function text in the original query.
!      */
!     tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosrc, &isnull);
!     if (isnull)
!         elog(ERROR, "null prosrc");
!     prosrc = DatumGetCString(DirectFunctionCall1(textout, tmp));
!
!     newerrposition = match_prosrc_to_query(prosrc, queryText, origerrposition);
!
!     if (newerrposition > 0)
!     {
!         /* Successful, so fix the error position */
!         errposition(newerrposition);
!     }
!     else
!     {
!         /*
!          * If unsuccessful, put in a CONTEXT entry that will cue clients
!          * not to treat the error position as relative to the original query.
!          */
!         errcontext("SQL function \"%s\"",
!                    NameStr(proc->proname));
!     }
! }
!
! /*
!  * Try to locate the string literal containing the function body in the
!  * given text of the CREATE FUNCTION command.  If successful, return the
!  * character (not byte) index within the command corresponding to the
!  * given character index within the literal.  If not successful, return 0.
!  */
! static int
! match_prosrc_to_query(const char *prosrc, const char *queryText,
!                       int cursorpos)
! {
!     /*
!      * Rather than fully parsing the CREATE FUNCTION command, we just scan
!      * the command looking for $prosrc$ or 'prosrc'.  This could be fooled
!      * (though not in any very probable scenarios), so fail if we find
!      * more than one match.
!      */
!     int        prosrclen = strlen(prosrc);
!     int        querylen = strlen(queryText);
!     int        matchpos = 0;
!     int        curpos;
!     int        newcursorpos;
!
!     for (curpos = 0; curpos < querylen-prosrclen; curpos++)
!     {
!         if (queryText[curpos] == '$' &&
!             strncmp(prosrc, &queryText[curpos+1], prosrclen) == 0 &&
!             queryText[curpos+1+prosrclen] == '$')
!         {
!             /*
!              * Found a $foo$ match.  Since there are no embedded quoting
!              * characters in a dollar-quoted literal, we don't have to do
!              * any fancy arithmetic; just offset by the starting position.
!              */
!             if (matchpos)
!                 return 0;        /* multiple matches, fail */
!             matchpos = pg_mbstrlen_with_len(queryText, curpos+1)
!                 + cursorpos;
!         }
!         else if (queryText[curpos] == '\'' &&
!                  match_prosrc_to_literal(prosrc, &queryText[curpos+1],
!                                          cursorpos, &newcursorpos))
!         {
!             /*
!              * Found a 'foo' match.  match_prosrc_to_literal() has adjusted
!              * for any quotes or backslashes embedded in the literal.
!              */
!             if (matchpos)
!                 return 0;        /* multiple matches, fail */
!             matchpos = pg_mbstrlen_with_len(queryText, curpos+1)
!                 + newcursorpos;
!         }
!     }
!
!     return matchpos;
! }
!
! /*
!  * Try to match the given source text to a single-quoted literal.
!  * If successful, adjust newcursorpos to correspond to the character
!  * (not byte) index corresponding to cursorpos in the source text.
!  *
!  * At entry, literal points just past a ' character.  We must check for the
!  * trailing quote.
!  */
! static bool
! match_prosrc_to_literal(const char *prosrc, const char *literal,
!                         int cursorpos, int *newcursorpos)
! {
!     int            newcp = cursorpos;
!     int            chlen;

      /*
!      * This implementation handles backslashes and doubled quotes in the
!      * string literal.  It does not handle the SQL syntax for literals
!      * continued across line boundaries.
       *
!      * We do the comparison a character at a time, not a byte at a time,
!      * so that we can do the correct cursorpos math.
       */
!     while (*prosrc)
!     {
!         cursorpos--;            /* characters left before cursor */
!         /*
!          * Check for backslashes and doubled quotes in the literal; adjust
!          * newcp when one is found before the cursor.
!          */
!         if (*literal == '\\')
!         {
!             literal++;
!             if (cursorpos > 0)
!                 newcp++;
!         }
!         else if (*literal == '\'')
!         {
!             if (literal[1] != '\'')
!                 return false;
!             literal++;
!             if (cursorpos > 0)
!                 newcp++;
!         }
!         chlen = pg_mblen(prosrc);
!         if (strncmp(prosrc, literal, chlen) != 0)
!             return false;
!         prosrc += chlen;
!         literal += chlen;
!     }
!
!     *newcursorpos = newcp;
!
!     if (*literal == '\'' && literal[1] != '\'')
!         return true;
!     return false;
  }
*** src/backend/commands/portalcmds.c.orig    Sat Nov 29 14:51:47 2003
--- src/backend/commands/portalcmds.c    Thu Mar 18 16:57:10 2004
***************
*** 270,275 ****
--- 270,276 ----
  PersistHoldablePortal(Portal portal)
  {
      QueryDesc  *queryDesc = PortalGetQueryDesc(portal);
+     Portal        saveActivePortal;
      MemoryContext savePortalContext;
      MemoryContext saveQueryContext;
      MemoryContext oldcxt;
***************
*** 311,316 ****
--- 312,319 ----
      /*
       * Set global portal context pointers.
       */
+     saveActivePortal = ActivePortal;
+     ActivePortal = portal;
      savePortalContext = PortalContext;
      PortalContext = PortalGetHeapMemory(portal);
      saveQueryContext = QueryContext;
***************
*** 342,347 ****
--- 345,351 ----
      /* Mark portal not active */
      portal->portalActive = false;

+     ActivePortal = saveActivePortal;
      PortalContext = savePortalContext;
      QueryContext = saveQueryContext;

*** src/backend/tcop/postgres.c.orig    Mon Mar 15 15:01:58 2004
--- src/backend/tcop/postgres.c    Thu Mar 18 16:56:55 2004
***************
*** 2710,2715 ****
--- 2710,2716 ----
           */
          MemoryContextSwitchTo(TopMemoryContext);
          MemoryContextResetAndDeleteChildren(ErrorContext);
+         ActivePortal = NULL;
          PortalContext = NULL;
          QueryContext = NULL;

*** src/backend/tcop/pquery.c.orig    Fri Mar  5 15:57:31 2004
--- src/backend/tcop/pquery.c    Thu Mar 18 16:56:56 2004
***************
*** 23,28 ****
--- 23,35 ----
  #include "utils/memutils.h"


+ /*
+  * ActivePortal is the currently executing Portal (the most closely nested,
+  * if there are several).
+  */
+ Portal    ActivePortal = NULL;
+
+
  static uint32 RunFromStore(Portal portal, ScanDirection direction, long count,
               DestReceiver *dest);
  static long PortalRunSelect(Portal portal, bool forward, long count,
***************
*** 395,400 ****
--- 402,408 ----
            char *completionTag)
  {
      bool        result;
+     Portal        saveActivePortal;
      MemoryContext savePortalContext;
      MemoryContext saveQueryContext;
      MemoryContext oldContext;
***************
*** 433,438 ****
--- 441,448 ----
      /*
       * Set global portal context pointers.
       */
+     saveActivePortal = ActivePortal;
+     ActivePortal = portal;
      savePortalContext = PortalContext;
      PortalContext = PortalGetHeapMemory(portal);
      saveQueryContext = QueryContext;
***************
*** 508,513 ****
--- 518,524 ----
      /* Mark portal not active */
      portal->portalActive = false;

+     ActivePortal = saveActivePortal;
      PortalContext = savePortalContext;
      QueryContext = saveQueryContext;

***************
*** 925,930 ****
--- 936,942 ----
                 DestReceiver *dest)
  {
      long        result;
+     Portal        saveActivePortal;
      MemoryContext savePortalContext;
      MemoryContext saveQueryContext;
      MemoryContext oldContext;
***************
*** 948,953 ****
--- 960,967 ----
      /*
       * Set global portal context pointers.
       */
+     saveActivePortal = ActivePortal;
+     ActivePortal = portal;
      savePortalContext = PortalContext;
      PortalContext = PortalGetHeapMemory(portal);
      saveQueryContext = QueryContext;
***************
*** 972,977 ****
--- 986,992 ----
      /* Mark portal not active */
      portal->portalActive = false;

+     ActivePortal = saveActivePortal;
      PortalContext = savePortalContext;
      QueryContext = saveQueryContext;

*** src/backend/utils/error/elog.c.orig    Mon Mar 15 15:01:58 2004
--- src/backend/utils/error/elog.c    Thu Mar 18 17:36:58 2004
***************
*** 808,813 ****
--- 808,829 ----
      return 0;                    /* return value does not matter */
  }

+ /*
+  * geterrposition --- return the currently set error position (0 if none)
+  *
+  * This is only intended for use in error callback subroutines, since there
+  * is no other place outside elog.c where the concept is meaningful.
+  */
+ int
+ geterrposition(void)
+ {
+     ErrorData  *edata = &errordata[errordata_stack_depth];
+
+     /* we don't bother incrementing recursion_depth */
+     CHECK_STACK_DEPTH();
+
+     return edata->cursorpos;
+ }

  /*
   * elog_finish --- finish up for old-style API
*** src/include/tcop/pquery.h.orig    Sat Nov 29 17:41:14 2003
--- src/include/tcop/pquery.h    Thu Mar 18 16:56:49 2004
***************
*** 17,22 ****
--- 17,25 ----
  #include "utils/portal.h"


+ extern DLLIMPORT Portal ActivePortal;
+
+
  extern void ProcessQuery(Query *parsetree,
               Plan *plan,
               ParamListInfo params,
*** src/include/utils/elog.h.orig    Mon Mar 15 15:02:09 2004
--- src/include/utils/elog.h    Thu Mar 18 17:36:51 2004
***************
*** 132,137 ****
--- 132,139 ----
  extern int    errfunction(const char *funcname);
  extern int    errposition(int cursorpos);

+ extern int    geterrposition(void);
+

  /*----------
   * Old-style error reporting API: to be used in this way: