[PATCHES] client side syntax error position (v3)
Dear patchers,
New submission... after suggestions by Tom and others, the current result
looks like the following:
psql> CREATE foo;
ERROR: syntax error at or near "foo" at character 8
LINE 1: CREATE foo;
^
psql>
There may still be issues about some mb encodings, especially with
localised error messages.
I think that the only consequence would be that the cursor may be placed
wrongly. Although it would be a bug, it is not a very big issue. I think
that the current patch may be considered for integration, and fixes for
some encodings can be added later if needed.
Also, any suggestion for improvement on this issue is obviously welcome,
but I need to be told the "how"...
Have a nice day,
--
Fabien Coelho - [EMAIL PROTECTED]*** ./src/bin/psql/common.c.origThu Mar 11 15:42:17 2004
--- ./src/bin/psql/common.c Fri Mar 12 09:24:13 2004
***
*** 345,350
--- 345,537
}
+ #define DISPLAY_SIZE (60)
+ #define MIN_RIGHT_CUT (10)
+ #define EXTRACT_FORMAT "LINE %d: "
+
+ /* on errors, print syntax error position if available.
+ * the query is expected to be in the client encoding.
+ */
+ static void
+ ReportSyntaxErrorPosition(const PGresult *result, const char *query)
+ {
+ int loc = 0;
+ char * sp = PQresultErrorField(result, PG_DIAG_STATEMENT_POSITION);
+
+ if (sp && sscanf(sp, "%d", &loc)!=1)
+ {
+ psql_error("INTERNAL ERROR: unexpected statement position '%s'\n", sp);
+ return;
+ }
+
+ /* do we have something to show? */
+ if (query!=NULL && loc>0)
+ {
+ int clen, slen, i, *qidx, ibeg, iend, last_nl,
+ loc_line, nb_lines, prefix_len, figure_len;
+ char *wquery, *cursor, *ex_prefix, *sp_prefix;
+ bool beg_trunc, end_trunc;
+
+ /* (1) let us first compute a character index for the query. */
+
+ /* we need a safe allocation size... */
+ slen = strlen(query);
+
+ /* the last one is needed to store last char mb length */
+ qidx = (int*) palloc(sizeof(int)*(slen+1));
+
+ qidx[0] = 0;
+ for (i=1; query[qidx[i-1]]!='\0' && i=0 && locloc)
+ iend = i;
+
+ wquery[qidx[i]] = ' ';
+ last_nl = i;
+ }
+ }
+ /* set extract beginning. */
+ if (i==loc && last_nl!=-1)
+ ibeg = last_nl+1;
+ }
+
+ /* (4) if the line extracted is too long, we truncate it. */
+ beg_trunc = false;
+ end_trunc = false;
+ if (iend-ibeg > DISPLAY_SIZE)
+ {
+ /* we first truncate right if it is enough. */
+ if (ibeg+DISPLAY_SIZE > loc+MIN_RIGHT_CUT)
+ {
+ iend = ibeg+DISPLAY_SIZE;
+ end_trunc = true;
+ }
+ else
+ {
+ /* we truncate right. */
+ if (loc+MIN_RIGHT_CUT < iend)
+ {
+ iend = loc+MIN_RIGHT_CUT;
+ end_trunc = true;
+ }
+
+ /* still too long?
+* we know that loc is at the end
+* (loc+MIN_RIGHT_CUT>=iend from above)
+* now we truncate left.
+*/
+ if (iend-ibeg > DISPLAY_SIZE)
+ {
+ ibeg = iend-DISPLAY_SIZE;
+ beg_trunc = true;
+ }
+ }
+ }
+
+ /* (5) if the line is too short, we could fix it here. */
+
+ /* the extract MUST contain the localisation! */
+ psql_assert(ibeg<=loc && loc=
+ prefix_len)
+ {
+ /* very unlikely. */
+ psql_error("INTERNAL ERROR: bad alloc fmt=\"%s\" len=%d
loc=%d\n",
+ EXTRACT_FORMAT, prefix_len, loc_line);
+ pfree(ex_prefix);
+ pfree(sp_prefix);
+ pfree(wquery);
+ pfree(qidx);
+ pfree(cursor);
+ return;
+ }
+
+ /* actual len used? mbstrlen? how do we know that the locale
+* translation is compatible w
Re: [PATCHES] defer statement logging until after parse
Bruce Momjian wrote: The problem I see with this patch is that it doesn't print the error query on a syntax error. That seems wrong. It does print it. In fact the example I gave below which is from a real trace shows it being printed. It is just printed after the error message rather than before. You solution doesn't appear to address the problem of what to do if they ask for only DDL and one of those generates a syntax error. cheers andrew I think you should print the query before parsing if they are asking for all queries to be logged, and print them after parsing if they want only DDL or DDL and data modification queries. I realize that duplicates some function calls, but I don't see any other way. --- Andrew Dunstan wrote: The attached patch is for review, not application. It defers logging statements until after they have been parsed. There should be no observable difference in behaviour if there is a successful parse, and on error the following traces appear: line:3 ERROR: syntax error at or near "se3d2" at character 1 line:4 LOG: parse error in statement: se3d2 aaa; Basically, I want to know that this will not break anything, and if so I will use it as the basis for a selective statement logging facility, based on the command tag(s) of the parsed statement(s), and incorporating Greg Stark's suggesion of a "syntax error" logging level. cheers andrew ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] notice about costly ri checks (3)
Dear Bruce, # [PATCHES] notice about costly ri checks (2), Fabien COELHO This one which appears on the web page can also be removed, It is also replaced by version 4. Sorry again for the inconvenience, have a nice day, -- Fabien Coelho - [EMAIL PROTECTED] ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] int8 fix for win32
Patch applied. Thanks. --- Claudio Natoli wrote: > > Fixes recent changes to test. For immediate application. > > [Note: int8-exp-three-digits.out needs the same treatment] > > Will review recent changes for float4/8 and implications for win32 when I > get a chance. > > > > > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED] -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] [HACKERS] notice about costly ri checks (3)
Fabien COELHO wrote: > > Dear Bruce, > > # [PATCHES] notice about costly ri checks (2), Fabien COELHO > > This one which appears on the web page can also be removed, > It is also replaced by version 4. > > Sorry again for the inconvenience, have a nice day, It is in the queue because I need that narrative for the commit messase. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] [HACKERS] notice about costly ri checks (3)
Fabien COELHO wrote: > > Dear Bruce, > > > It is in the queue because I need that narrative for the commit > > messase. > > Ok, I get the point. > > I should have made a fully standalone version with all comments repeated. > > Thanks, have a nice day, No problem. I am used to keeping multiple versions for the comments. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] defer statement logging until after parse
Andrew Dunstan wrote: > > > Bruce Momjian wrote: > > >The problem I see with this patch is that it doesn't print the error > >query on a syntax error. That seems wrong. > > > > It does print it. In fact the example I gave below which is from a real > trace shows it being printed. It is just printed after the error message > rather than before. > > You solution doesn't appear to address the problem of what to do if they > ask for only DDL and one of those generates a syntax error. My comment was that if they type "UP8ATE", and it is a syntax error, we have no way to know if it was a DDL or not, so we don't print it. My idea was to take log_statement, and instead of true/false, have it be all, ddl, mod, or off/none/false(?). You keep the existing test for log_statement where it is, but test for 'all' now, and after parse, you check for ddl or mod, and print in those cases if the tag matches. If they want ddl and errors, they can use log_min_error_statement to see just statement error, and set log_statement accordingly. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] [HACKERS] notice about costly ri checks (3)
Dear Bruce, > It is in the queue because I need that narrative for the commit > messase. Ok, I get the point. I should have made a fully standalone version with all comments repeated. Thanks, have a nice day, -- Fabien Coelho - [EMAIL PROTECTED] ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] defer statement logging until after parse
Bruce Momjian said: > Andrew Dunstan wrote: >> >> >> Bruce Momjian wrote: >> >> >The problem I see with this patch is that it doesn't print the error >> >query on a syntax error. That seems wrong. >> > >> >> It does print it. In fact the example I gave below which is from a >> real trace shows it being printed. It is just printed after the error >> message rather than before. >> >> You solution doesn't appear to address the problem of what to do if >> they ask for only DDL and one of those generates a syntax error. > > My comment was that if they type "UP8ATE", and it is a syntax error, we > have no way to know if it was a DDL or not, so we don't print it. > > My idea was to take log_statement, and instead of true/false, have it > be all, ddl, mod, or off/none/false(?). You keep the existing test for > log_statement where it is, but test for 'all' now, and after parse, you > check for ddl or mod, and print in those cases if the tag matches. > > If they want ddl and errors, they can use log_min_error_statement to > see just statement error, and set log_statement accordingly. > The problem is that you are anticipating my solution for the selectivity issue before I have written or submitted it. My question was different and narrower - namely will the patch I sent, as it stands, and forgetting the selectivity issue for the moment, break anything? When I actually send in a patch to implement statement log selectivity, I will give you free license to pull it to bits to your heart's content. cheers andrew ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] defer statement logging until after parse
Bruce Momjian <[EMAIL PROTECTED]> writes: > My idea was to take log_statement, and instead of true/false, have it be > all, ddl, mod, or off/none/false(?). You keep the existing test for > log_statement where it is, but test for 'all' now, and after parse, you > check for ddl or mod, and print in those cases if the tag matches. Has any of this discussion taken into account the fact that a querystring may contain multiple commands? regards, tom lane ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] defer statement logging until after parse
Andrew Dunstan <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> Has any of this discussion taken into account the fact that a >> querystring may contain multiple commands? > What does the parser do if one of the statements has an error and the > others are OK? The whole thing is rejected. This is just an instance of the general rule that processing of the entire querystring is abandoned at the first error. The current definition of log_statement has no problem because we print the whole string, once, before parsing starts. If you put a printout into the per-parse-tree loop then I think you are going to get multiple printouts of the same string. You could add some state to prevent more than one printout per querystring, but even then you'll get complaints "I asked for DDL only, why did it print this SELECT?". ISTM the only way to make it "work" without obvious implementation artifacts is to actually break down the string into individual commands, which is more work than I think this feature is worth. regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] defer statement logging until after parse
Tom Lane wrote: The current definition of log_statement has no problem because we print the whole string, once, before parsing starts. If you put a printout into the per-parse-tree loop then I think you are going to get multiple printouts of the same string. I didn't intend to - I intended to do it before that. All or nothing deal. e.g. if they want DML then if any of the queries has insert/update/delete/copy they get the whole query string. You could add some state to prevent more than one printout per querystring, but even then you'll get complaints "I asked for DDL only, why did it print this SELECT?". ISTM the only way to make it "work" without obvious implementation artifacts is to actually break down the string into individual commands, which is more work than I think this feature is worth. Well, it gets worse than that when you consider that I tend to put all my DML inside a stored proc and have the client call "select myproc(args)". However, people have asked for a facility to filter out stuff, especially to filter out select statements they are not interested in. I agree that it is simpleminded, and I wouldn't use it. But we can put warnings about likely effects in the docs. Or we can abandon the whole idea and remove it from the TODO list. I'm not burning up with desire to do this. cheers andrew ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] [pgsql-hackers-win32] initdb problen
I believe that the attached patch may solve this setlocale() problem,
but I do not have a Windows box handy on which to test it. Can somebody
who does please try and let us know the results?
thanks
andrew
Andrew Dunstan wrote:
the answer is very probably here:
http://archives.postgresql.org/pgsql-hackers-win32/2003-10/msg00024.php
I thought Bruce said he had this covered, but it hasn't been fixed. I
will prepare a patch in the next day or so if someone else doesn't beat
me to it.
cheers
andrew
Korea PostgreSQL Users' Group wrote:
it works still problem.
setlocale() function is not equivalent between unix and win32
plz check.
multibyte text is not sorted.
- Original Message -
From: "Claudio Natoli" <[EMAIL PROTECTED]>
To: "'Korea PostgreSQL Users' Group'" <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>
Sent: Friday, March 12, 2004 5:04 PM
Subject: RE: [pgsql-hackers-win32] initdb problen
What if you instead try:
initdb --locale=C
Looks like "--no-locale" is not "equivalent to --locale=C", at least under
win32.
Cheers,
Claudio
---(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
---(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
Index: src/backend/main/main.c
===
RCS file: /projects/cvsroot/pgsql-server/src/backend/main/main.c,v
retrieving revision 1.75
diff -c -r1.75 main.c
*** src/backend/main/main.c 5 Mar 2004 01:11:04 - 1.75
--- src/backend/main/main.c 12 Mar 2004 16:27:13 -
***
*** 74,79
--- 74,83
#endif /* NOPRINTADE */
#endif /* __alpha */
+ #ifdef WIN32
+ char *env_locale;
+ #endif
+
#if defined(NOFIXADE) || defined(NOPRINTADE)
#if defined(ultrix4)
***
*** 143,150
--- 147,176
* set later during GUC option processing, but we set it here to allow
* startup error messages to be localized.
*/
+
+ #ifdef WIN32
+ /*
+* Windows uses codepages rather than the environment, so we work around
+* that by querying the environment explicitly first for LC_COLLATE
+* and LC_CTYPE. We have to do this because initdb passes those values
+* in the environment. If there is nothing there we fall back on the
+* codepage.
+*/
+
+ if ((env_locale = getenv("LC_COLLATE")) != NULL)
+ setlocale(LC_COLLATE,env_locale);
+ else
+ setlocale(LC_COLLATE, "");
+
+ if ((env_locale = getenv("LC_CTYPE")) != NULL)
+ setlocale(LC_CTYPE,env_locale);
+ else
+ setlocale(LC_CTYPE, "");
+ #else
setlocale(LC_COLLATE, "");
setlocale(LC_CTYPE, "");
+ #endif
+
#ifdef LC_MESSAGES
setlocale(LC_MESSAGES, "");
#endif
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] Update tests & memory leak fix
Bruce, do you have an update on when this patch (and the subsequent one) will be applied or rejected? Thanks, Lee. Bruce Momjian writes Mon, 8 Mar 2004: > > Got the diff -c now, thanks. > > --- > > Lee Kindness wrote: > > Sorry guys, last diff wasn't a context one! "diff -c" attached this time > > round... > > > > L. > > > > - Original Message - > > From: Lee Kindness > > To: [EMAIL PROTECTED] > > Cc: Bruce Momjian ; Lee Kindness ; Shridhar Daithankar > > Sent: Saturday, March 06, 2004 3:07 PM > > Subject: ECPG: Update tests & memory leak fix > > > > See attached diff which corrects a per-thread memory leak in libecpg and > > changes the thread test program to create a user defined number of threads. > > Also attached tar.gz adds an additional thread test, which currently does > > NOT work as expected and is one of the cases Shridar and I will be looking > > at (removing need for AT xxx on every EXEC SQL). > > > > Note, the updated test shows that ECPG currently DOES support the > > specification of connection names by host variables (not only static strings > > as I had said previously). > > > > Thanks, L. > > [ Attachment, skipping... ] > > > > > ---(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 > > -- > Bruce Momjian| http://candle.pha.pa.us > [EMAIL PROTECTED] | (610) 359-1001 > + If your life is a hard drive, | 13 Roberts Road > + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] Update tests & memory leak fix
I will put it in the queue today. --- Lee Kindness wrote: > Bruce, do you have an update on when this patch (and the subsequent > one) will be applied or rejected? > > Thanks, Lee. > > Bruce Momjian writes Mon, 8 Mar 2004: > > > > Got the diff -c now, thanks. > > > > --- > > > > Lee Kindness wrote: > > > Sorry guys, last diff wasn't a context one! "diff -c" attached this time > > > round... > > > > > > L. > > > > > > - Original Message - > > > From: Lee Kindness > > > To: [EMAIL PROTECTED] > > > Cc: Bruce Momjian ; Lee Kindness ; Shridhar Daithankar > > > Sent: Saturday, March 06, 2004 3:07 PM > > > Subject: ECPG: Update tests & memory leak fix > > > > > > See attached diff which corrects a per-thread memory leak in libecpg and > > > changes the thread test program to create a user defined number of threads. > > > Also attached tar.gz adds an additional thread test, which currently does > > > NOT work as expected and is one of the cases Shridar and I will be looking > > > at (removing need for AT xxx on every EXEC SQL). > > > > > > Note, the updated test shows that ECPG currently DOES support the > > > specification of connection names by host variables (not only static strings > > > as I had said previously). > > > > > > Thanks, L. > > > > [ Attachment, skipping... ] > > > > > > > > ---(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 > > > > -- > > Bruce Momjian| http://candle.pha.pa.us > > [EMAIL PROTECTED] | (610) 359-1001 > > + If your life is a hard drive, | 13 Roberts Road > > + Christ can be your backup.| Newtown Square, Pennsylvania 19073 > -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(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] defer statement logging until after parse
Andrew Dunstan wrote: > >> It does print it. In fact the example I gave below which is from a > >> real trace shows it being printed. It is just printed after the error > >> message rather than before. > >> > >> You solution doesn't appear to address the problem of what to do if > >> they ask for only DDL and one of those generates a syntax error. > > > > My comment was that if they type "UP8ATE", and it is a syntax error, we > > have no way to know if it was a DDL or not, so we don't print it. > > > > My idea was to take log_statement, and instead of true/false, have it > > be all, ddl, mod, or off/none/false(?). You keep the existing test for > > log_statement where it is, but test for 'all' now, and after parse, you > > check for ddl or mod, and print in those cases if the tag matches. > > > > If they want ddl and errors, they can use log_min_error_statement to > > see just statement error, and set log_statement accordingly. > > > > The problem is that you are anticipating my solution for the selectivity > issue before I have written or submitted it. My question was different and > narrower - namely will the patch I sent, as it stands, and forgetting the > selectivity issue for the moment, break anything? > > When I actually send in a patch to implement statement log selectivity, I > will give you free license to pull it to bits to your heart's content. Well, if that is the question, then I don't want to reorder the query printout from the error. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] defer statement logging until after parse
Tom Lane wrote: > Andrew Dunstan <[EMAIL PROTECTED]> writes: > > Tom Lane wrote: > >> Has any of this discussion taken into account the fact that a > >> querystring may contain multiple commands? > > > What does the parser do if one of the statements has an error and the > > others are OK? > > The whole thing is rejected. This is just an instance of the general > rule that processing of the entire querystring is abandoned at the first > error. > > The current definition of log_statement has no problem because we print > the whole string, once, before parsing starts. If you put a printout > into the per-parse-tree loop then I think you are going to get multiple > printouts of the same string. You could add some state to prevent more > than one printout per querystring, but even then you'll get complaints > "I asked for DDL only, why did it print this SELECT?". ISTM the only > way to make it "work" without obvious implementation artifacts is to > actually break down the string into individual commands, which is more > work than I think this feature is worth. I think it is acceptable to print the entire query string once if one part of it has a DDL or mod statement. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] defer statement logging until after parse
Bruce Momjian wrote: Well, if that is the question, then I don't want to reorder the query printout from the error. OK. I'll let someone else do it. I have no need for it. Forget I spoke. cheers andrew ---(end of broadcast)--- TIP 8: explain analyze is your friend
