Skip site navigation (1) Skip section navigation (2)

Re: syntax error position "CREATE FUNCTION" bug fix

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
Cc: PostgreSQL Patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: syntax error position "CREATE FUNCTION" bug fix
Date: 2004-03-18 23:33:49
Message-ID: 14229.1079652829@sss.pgh.pa.us (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-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:

In response to

Responses

pgsql-hackers by date

Next:From: Larry RosenmanDate: 2004-03-19 00:05:35
Subject: Re: UnixWare/CVS Tip/initdb.c needs to use threads flags...
Previous:From: Bruce MomjianDate: 2004-03-18 23:28:22
Subject: Re: Will auto-cluster be in 7.5?

pgsql-patches by date

Next:From: Larry RosenmanDate: 2004-03-19 00:05:35
Subject: Re: UnixWare/CVS Tip/initdb.c needs to use threads flags...
Previous:From: Bruce MomjianDate: 2004-03-18 23:27:39
Subject: Re: UnixWare Thread Patch (and test_fsync)

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group