Re: syntax error position "CREATE FUNCTION" bug fix

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: