pg.ddx.io pgsql-interfaces@postgresql.org mailing list archive
help / color / mirror / Atom feedProposed patch for error locations
7+ messages / 5 participants
[nested] [flat]
* Proposed patch for error locations
@ 2006-03-12 17:37 Tom Lane <tgl@sss.pgh.pa.us>
2006-03-13 07:42 ` Re: Proposed patch for error locations Michael Glaesemann <grzm@myrealbox.com>
2006-03-13 11:43 ` Re: Proposed patch for error locations Martijn van Oosterhout <kleptog@svana.org>
2006-03-13 14:22 ` Re: Proposed patch for error locations Fabien COELHO <coelho@cri.ensmp.fr>
0 siblings, 3 replies; 7+ messages in thread
From: Tom Lane @ 2006-03-12 17:37 UTC (permalink / raw)
To: pgsql-patches@postgreSQL.org
The attached WIP patch improves the parser so that cursor positions can be
attached to many errors reported during parse analysis, such as complaints
about nonexistent columns or functions. This is along the lines proposed
in my rant here:
http://archives.postgresql.org/pgsql-patches/2006-02/msg00234.php
to wit, that we make use of bison's "locations" feature to track token
positions in the grammar, and add locations to raw parse tree nodes as
needed.
Some examples of what it can do:
regression=# select unique1,unique2,tem,hundred from tenk1 t;
ERROR: column "tem" does not exist at character 24
LINE 1: select unique1,unique2,tem,hundred from tenk1 t;
^
regression=# select unique1+unique2,ten+hundred,thousand+stringu1 from tenk1;
ERROR: operator does not exist: integer + name at character 44
HINT: No operator matches the given name and argument type(s). You may need to add explicit type casts.
LINE 1: select unique1+unique2,ten+hundred,thousand+stringu1 from te...
^
These examples are perhaps not all that exciting, but in a complex SQL
statement extending over dozens of lines I think it's really important
to be able to do this.
I haven't gone further than making it finger the locations of bogus column
references, operators, and functions --- it's probably worth improving
TypeCast and maybe a few other raw-parse-tree constructs too. Another thing
that needs to be looked at is using the mechanism to locate statement
boundaries, which was the starting-point of the thread mentioned above.
One thing I'm noticing already is that the addition of "at character N"
to a lot of these messages isn't an improvement. In psql it's certainly
redundant with the error-cursor display. As I recall, we include that string
in the primary message text to improve usability with poorer application APIs
that might only show the primary error message to the user. I'm thinking
maybe it's time to expect those client libraries to do better. Any thoughts
about that?
Any comments in general?
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Proposed patch for error locations
2006-03-12 17:37 Proposed patch for error locations Tom Lane <tgl@sss.pgh.pa.us>
@ 2006-03-13 07:42 ` Michael Glaesemann <grzm@myrealbox.com>
2006-03-13 15:11 ` Re: [PATCHES] Proposed patch for error locations Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 7+ messages in thread
From: Michael Glaesemann @ 2006-03-13 07:42 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-patches@postgreSQL.org
On Mar 13, 2006, at 2:37 , Tom Lane wrote:
> to wit, that we make use of bison's "locations" feature to track token
> positions in the grammar, and add locations to raw parse tree nodes as
> needed.
>
> Some examples of what it can do:
This looks really nice.
> One thing I'm noticing already is that the addition of "at
> character N"
> to a lot of these messages isn't an improvement. In psql it's
> certainly
> redundant with the error-cursor display.
The pure character count is definitely difficult to use with larger
queries. I think it could be more useful if it were
line:char_of_line. Would others find this useful?
Michael Glaesemann
grzm myrealbox com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [PATCHES] Proposed patch for error locations
2006-03-12 17:37 Proposed patch for error locations Tom Lane <tgl@sss.pgh.pa.us>
2006-03-13 07:42 ` Re: Proposed patch for error locations Michael Glaesemann <grzm@myrealbox.com>
@ 2006-03-13 15:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2006-03-14 02:53 ` Re: [PATCHES] Proposed patch for error locations Christopher Kings-Lynne <chriskl@familyhealth.com.au>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2006-03-13 15:11 UTC (permalink / raw)
To: Michael Glaesemann <grzm@myrealbox.com>; +Cc: pgsql-patches@postgreSQL.org; pgsql-interfaces
[ cross-posting to pgsql-interfaces in hopes of drawing more comments ]
Michael Glaesemann <grzm@myrealbox.com> writes:
> On Mar 13, 2006, at 2:37 , Tom Lane wrote:
>> http://archives.postgresql.org/pgsql-patches/2006-03/msg00153.php
> This looks really nice.
>> One thing I'm noticing already is that the addition of "at character N"
>> to a lot of these messages isn't an improvement. In psql it's
>> certainly redundant with the error-cursor display.
> The pure character count is definitely difficult to use with larger
> queries. I think it could be more useful if it were
> line:char_of_line. Would others find this useful?
The change in behavior would actually be in libpq, because it's
PQerrorMessage that is doing the deed (assuming a reasonably modern
server and libpq). What I was considering proposing was that we migrate
the error-cursor feature out of psql and into PQerrorMessage. This
would mean that you'd get responses like
ERROR: column "foo" does not exist
LINE 1: select foo from a;
^
from all libpq-using applications not just psql. We could make this
conditional on the error verbosity --- in "terse" mode the "LINE N"
output wouldn't appear, and "at character N" still would. Applications
should already be expecting multiline outputs from PQerrorMessage if
they're in non-terse mode, so this ought to be OK. Comments?
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [PATCHES] Proposed patch for error locations
2006-03-12 17:37 Proposed patch for error locations Tom Lane <tgl@sss.pgh.pa.us>
2006-03-13 07:42 ` Re: Proposed patch for error locations Michael Glaesemann <grzm@myrealbox.com>
2006-03-13 15:11 ` Re: [PATCHES] Proposed patch for error locations Tom Lane <tgl@sss.pgh.pa.us>
@ 2006-03-14 02:53 ` Christopher Kings-Lynne <chriskl@familyhealth.com.au>
0 siblings, 0 replies; 7+ messages in thread
From: Christopher Kings-Lynne @ 2006-03-14 02:53 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Glaesemann <grzm@myrealbox.com>; pgsql-patches@postgresql.org; pgsql-interfaces
> from all libpq-using applications not just psql. We could make this
> conditional on the error verbosity --- in "terse" mode the "LINE N"
> output wouldn't appear, and "at character N" still would. Applications
> should already be expecting multiline outputs from PQerrorMessage if
> they're in non-terse mode, so this ought to be OK. Comments?
Sounds like it'd be handy in phpPgAdmin...
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Proposed patch for error locations
2006-03-12 17:37 Proposed patch for error locations Tom Lane <tgl@sss.pgh.pa.us>
@ 2006-03-13 11:43 ` Martijn van Oosterhout <kleptog@svana.org>
2006-03-13 15:44 ` Re: Proposed patch for error locations Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 7+ messages in thread
From: Martijn van Oosterhout @ 2006-03-13 11:43 UTC (permalink / raw)
To: pgsql-patches@postgresql.org
> The attached WIP patch improves the parser so that cursor positions
> can be attached to many errors reported during parse analysis, such
> as complaints about nonexistent columns or functions. This is along
> the lines proposed in my rant here:
> http://archives.postgresql.org/pgsql-patches/2006-02/msg00234.php
> to wit, that we make use of bison's "locations" feature to track
> token positions in the grammar, and add locations to raw parse tree
> nodes as needed.
My only comment is: Wouldn't it be clearer in the long run to create a
ParseNode base structure which is used in place of NodeTag for
parsenodes, and put the "int location" in there.
Then you could create a new function called makeParseNode which works
like makeNode but takes an extra argument giving the location. Or some
variation of the two.
The thing I'm wondering is whether there will ever be any other
information we want to store, like token length so you can bracket the
expression and/or extract the exact expression with the issue. For
example, the (start,end) of a function call includes everything within
the brackets. By abstrating into a makeParseNode function, you may be
able to avoid changing a lot of code down the line.
Finally, is the count in characters or bytes (w.r.t. multibyte
encodings)?
Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.
Attachments:
[application/pgp-signature] signature.asc (188B, ../../20060313114331.GC6714@svana.org/2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Proposed patch for error locations
2006-03-12 17:37 Proposed patch for error locations Tom Lane <tgl@sss.pgh.pa.us>
2006-03-13 11:43 ` Re: Proposed patch for error locations Martijn van Oosterhout <kleptog@svana.org>
@ 2006-03-13 15:44 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 7+ messages in thread
From: Tom Lane @ 2006-03-13 15:44 UTC (permalink / raw)
To: Martijn van Oosterhout <kleptog@svana.org>; +Cc: pgsql-patches@postgresql.org
Martijn van Oosterhout <kleptog@svana.org> writes:
> My only comment is: Wouldn't it be clearer in the long run to create a
> ParseNode base structure which is used in place of NodeTag for
> parsenodes, and put the "int location" in there.
Only if we really wanted *every* parse node to carry a location, which
I think is overkill. If we took it far enough we'd be needing location
decoration on List nodes too, and that is definitely not happening.
There's a tradeoff here between completeness of information and
performance. In some quick tests with pgbench I don't detect any
noticeable performance loss with the patch as proposed, but it'd be
very easy to cross the threshold to where it does cost performance,
if we get too excited about decorating parse nodes with more than a
minimal amount of overhead.
> Finally, is the count in characters or bytes (w.r.t. multibyte
> encodings)?
Bytes; see the comments. Converting to a character count only happens
if an actual error message has to be emitted. (This is uglier than I
would like, but I don't see any sufficiently-cheap way to get the lexer
to count multibyte characters correctly.)
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Proposed patch for error locations
2006-03-12 17:37 Proposed patch for error locations Tom Lane <tgl@sss.pgh.pa.us>
@ 2006-03-13 14:22 ` Fabien COELHO <coelho@cri.ensmp.fr>
2 siblings, 0 replies; 7+ messages in thread
From: Fabien COELHO @ 2006-03-13 14:22 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: PostgreSQL Patches <pgsql-patches@postgresql.org>
Dear Tom,
> I haven't gone further than making it finger the locations of bogus column
> references, operators, and functions --- it's probably worth improving
> TypeCast and maybe a few other raw-parse-tree constructs too.
Yes. Other area would also benefit, such as type names:
psql: CREATE FUNCTION foo() RETURNS INTEGET LANGUAGE plpgsql AS ...
ERROR: type "integet" does not exist
That is a kind of "entry level" user features which are good for my
students ... as well as myself. Thus I'm looking forward to have more help
along those lines, although I cannot give a hand at the time.
--
Fabien.
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2006-03-14 02:53 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2006-03-12 17:37 Proposed patch for error locations Tom Lane <tgl@sss.pgh.pa.us>
2006-03-13 07:42 ` Michael Glaesemann <grzm@myrealbox.com>
2006-03-13 15:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2006-03-14 02:53 ` Christopher Kings-Lynne <chriskl@familyhealth.com.au>
2006-03-13 11:43 ` Martijn van Oosterhout <kleptog@svana.org>
2006-03-13 15:44 ` Tom Lane <tgl@sss.pgh.pa.us>
2006-03-13 14:22 ` Fabien COELHO <coelho@cri.ensmp.fr>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox