Re: [PATCHES] syntax error position "CREATE FUNCTION" bug fix
Dear Tom,
> Attached is a proposed patch that fixes the
> cursor-position-in-CREATE-FUNCTION issue per my earlier suggestion.
>
> The re-parsing of the original command is simplistic but will handle all
> normal cases.
> [...]
That's quite a demonstration;-)
However, I still stick with my "bad" simple idea because the simpler the
better, and also because of the following example:
psql> CREATE OR REPLACE FUNCTION count_tup(TEXT) RETURNS INTEGER AS '
DECLARE
n RECORD;
BEGIN
FOR n IN EXECUTE \'SELECT COUNT(*) AS c FRM \' || $1
LOOP
RETURN n.c;
END LOOP;
RETURN NULL;
END;'
LANGUAGE plpgsql;
psql> SELECT count_tup('pg_shadow');
ERROR: syntax error at or near "FRM" at character 22
CONTEXT: PL/pgSQL function "count_tup" line 4 at for over execute statement
As you can notice, the extract is not in the submitted query, so there
is no point to show it there.
Maybe real PL/pgSQL programmers will never have syntax errors with their
SQL stuff.
Thus I really think that the parser should return the processed query,
at least in some cases.
Anyway, have a nice day!
--
Fabien Coelho - [EMAIL PROTECTED]
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] 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 intmatch_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;
! boolisnull;
! 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 */
Re: [PATCHES] syntax error position "CREATE FUNCTION" bug fix
Fabien COELHO <[EMAIL PROTECTED]> writes: >> But it cannot (easily) match it up with the *original input* > Sure. Even the parser in the backend cannot do it, that's the problem!;-) We *can* do it, it's just a matter of the pain level. For instance we could have the main lexer save aside an indication of the number of quoting characters it discarded at each position. The problem in my mind is to not put a heavy overhead on the main lexing path for information that will only get used in an error case. I was toying with the idea of having the CREATE FUNCTION error callback go back to the original command string (which I believe is now always saved in the Portal, and certainly could be if it isn't) and re-parse it using a slower variant of the lexer that keeps this info. Not sure what all is involved in that approach, but in principle this would let us meet the original design goal without any overhead except in the error case. > Would you accept a "it works sometimes, but it may be wrong others" hack? I think it'd be okay if it gets the common cases right and doesn't generate lies in the cases it can't get right. Also, I think it might be an acceptable compromise to give the position correctly only when the function body is quoted using dollar-quoting. (Everybody's gonna be using dollar quoting to write their functions in 7.5, right? ;-)) In that case it is trivial to convert the "inside" syntax error offset to an offset in the original string literal, and you only have to know the position of that literal in the original command to finish up. BTW, it's worth pointing out that the client will have to special-case the situation where a CONTEXT entry is present anyhow, since that most likely means that the syntax error is inside some function called by the query, and not in the query itself. So the hack in psql doesn't go away in any case; all that we do differently is not send CONTEXT from the SQL function parse error callback if we are able to convert the syntax error offset to something relative to the original command. (One could envision a really smart client pulling back the text of the function identified by the topmost CONTEXT entry and putting the cursor on that --- of course this would have to be in a popup window not the original query, but it's doable in principle.) >> My idea of a GUI syntax error report is something that puts my editing >> cursor in the right place. > Thus you decided that you prefer that NO interface should be able to show > the correct position, rather than having at least one to do it, and other > being able to display something, because you decided that the only place > to show something in a GUI is in the initial window or never. No, I say that we shouldn't put in a kluge that gets it sort-of right in a simple interface and makes it impossible for better interfaces to get it really right. I think we can do better than that. regards, tom lane ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] syntax error position "CREATE FUNCTION" bug fix
Dear Tom, > > Any GUI can take advantage of the returned string to show it in a window > > with fancy colors and do any hilighting around the position. > > But it cannot (easily) match it up with the *original input* Sure. Even the parser in the backend cannot do it, that's the problem!;-) > and put the cursor in the right place in the *input* window. You are > envisioning something that's no better than psql with window borders. I try to envision what is achievable with a reasonnable effort;-) If I read you correctly, it is all interfaces or none... As a mostly; psql user, I'm not lucky;-) I don't think it is really easy to compute the good position wrt to the original query if you want to keep into account escapes that are eaten by the first parsing. I can provide a fix that would catch simple cases, but not all of them. Would you accept a "it works sometimes, but it may be wrong others" hack? > My idea of a GUI syntax error report is something that puts my editing > cursor in the right place. Thus you decided that you prefer that NO interface should be able to show the correct position, rather than having at least one to do it, and other being able to display something, because you decided that the only place to show something in a GUI is in the initial window or never. You don't like dialog box, I guess;-) Have a nice day, -- Fabien Coelho - [EMAIL PROTECTED] ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] syntax error position "CREATE FUNCTION" bug fix
Fabien COELHO <[EMAIL PROTECTED]> writes: >> Moreover, I don't like the proposed protocol change anyway. This >> approach only solves the problem for psql-like error reporting, namely >> clients that are going to regurgitate a string in their error message >> and aren't very picky about whether that string really matches the >> original input. One of the design goals for the error reporting feature >> is that GUI-type clients should be able to mark syntax error positions >> by highlighting in the original input window. This proposal abandons >> that goal. > I cannot see your point. > Any GUI can take advantage of the returned string to show it in a window > with fancy colors and do any hilighting around the position. But it cannot (easily) match it up with the *original input* and put the cursor in the right place in the *input* window. You are envisioning something that's no better than psql with window borders. My idea of a GUI syntax error report is something that puts my editing cursor in the right place. > I've implemented the stuff for psql (with your help btw), but I cannot see > why it couldn't be used with other interfaces? It's not the right thing for other interfaces. If it were the right thing for all interfaces, we'd have put the functionality in the backend to begin with. regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] syntax error position "CREATE FUNCTION" bug fix
Dear Tom, > > Please find attached a patch to fix the "CREATE FUNCTION" syntax error > > position bug I reported a few days ago. > > This strikes me as much too intrusive to be worthwhile ... the problem > is simply not important enough to justify a protocol change. Then I can suggest to happend the string after the position. NO protocol change, but the convention that the string is shown after the position. Small difference, I sent a proposal later. > Moreover, I don't like the proposed protocol change anyway. This > approach only solves the problem for psql-like error reporting, namely > clients that are going to regurgitate a string in their error message > and aren't very picky about whether that string really matches the > original input. One of the design goals for the error reporting feature > is that GUI-type clients should be able to mark syntax error positions > by highlighting in the original input window. This proposal abandons > that goal. I cannot see your point. Any GUI can take advantage of the returned string to show it in a window with fancy colors and do any hilighting around the position. I've implemented the stuff for psql (with your help btw), but I cannot see why it couldn't be used with other interfaces? The issue is that the error position is not enough in some cases. I proposed the only possible fix for that, which is to provide the missing information to the client, which will do something or nothing about it. The current status is that the information is not available, so nothing can be done. Moreover, I had problems with syntax errors in string-embedded queries in the past, and this is trickier to solve, so I don't see why the error position should not be fixed for those errors. So my point is that I agree that the protocole should not be changed, but I disagree that the "bug" should remain as it is because of some principles. Have a nice day, -- Fabien Coelho - [EMAIL PROTECTED] ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] syntax error position "CREATE FUNCTION" bug fix
Fabien COELHO <[EMAIL PROTECTED]> writes: > Please find attached a patch to fix the "CREATE FUNCTION" syntax error > position bug I reported a few days ago. This strikes me as much too intrusive to be worthwhile ... the problem is simply not important enough to justify a protocol change. Moreover, I don't like the proposed protocol change anyway. This approach only solves the problem for psql-like error reporting, namely clients that are going to regurgitate a string in their error message and aren't very picky about whether that string really matches the original input. One of the design goals for the error reporting feature is that GUI-type clients should be able to mark syntax error positions by highlighting in the original input window. This proposal abandons that goal. regards, tom lane ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
