Re: Syntax error reporting (was Re: [PATCHES] syntax error position

Dear Tom,

> > However, I still stick with my "bad" simple idea because the simpler the
> > better, and also because of the following example:
> > ...
> > 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.
>
> Yeah. However, I dislike your solution because it confuses the cases of
> a syntax error in the actually submitted query, and a syntax error in an
> internally generated query. We should keep these cases clearly separate
> because clients may want to do different things.
> [...]

I agree with you that both reports should not look the same.

The good news is that they already do not look the same, thanks
to the CONTEXT information. However, the context information is
informal (it is just plain English), to it is not easy for a client
to take that into account.

> The original design concept for the 'P' (position) error field is that
> it would be used to locate syntax errors in the *original query*, and
> so its presence is a cue to the client code to go in the direction of
> setting the editing cursor. (Note the protocol specification says
> "index into the original query string".)

Yes, I noted that.

In my proposed patch, I changed it to the "processed" query, which
may or may not be the initial query.

> We have in fact misimplemented it, because it is being set for syntax
> errors in internally generated queries too.

Well, from the parser point of view, it is a little bit messy to have
to do different things for error reporting in different context. That
why I would try a one-fit-all solution. Maybe I'm a little bit lazy, but
sometimes it is a quality in programming, if it help keep things simple.

> I was already planning to modify plpgsql to send back the full text of
> generated queries when there is an error. My intention was to supply
> this just as part of the CONTEXT stack, that is instead of your example
> of
>
> ERROR: syntax error at or near "FRM" at character 22
> CONTEXT: PL/pgSQL function "count_tup" line 4 at for over execute statement
>
> you'd get something like
>
> ERROR: syntax error at or near "FRM" at character 22
> CONTEXT: Executing command "SELECT COUNT(*) AS c FRM pg_shadow"
> PL/pgSQL function "count_tup" line 4 at for over execute statement
>
> However it might be better to invent a new error-message field that
> carries just the text of the SQL command, rather than stuffing it into
> CONTEXT.

I'm not sure I would like the CONTEXT field for that? as it may be
usefull for a lot of things. In your above example, the CONTEXT is filled
with 2 different informations that are just messed up for the client.
If localisation is used, there would be no way for a client to seperate
them. Different information should require different fields.

More over, I have other ideas for CONTEXT, which should really be a stack.
But that is another issue.

> (This is similar to your original patch, but different in detail because
> I'm envisioning sending back generated queries, never the submitted
> query. Regurgitating the submitted query is just a waste of bandwidth.)

Well, if it is the same I agree. On the other hand, it is a rare case,
during an exception. I would expect submitted queries to work most of the
time, because the client is just some program which uses the database.

> The plus side of that would be that it'd be easy to extract
> for syntax-error highlighting. The minuses are that existing clients
> would fail to print such a field (the protocol spec says to ignore
> unknown fields

That's a really good design idea, because it allows to include new fields
and still to have old client compatible with the new protocol.

I think it is no big deal if existing clients don't make use if the new
information. It was not there before anyway, so it is just as is was
before.

Moreover, one should distinguish between fields for the human and fields
for the client. reporting the query and the position is more for the
client, giving the context is more for the human. One is use for
some automatic processing, the other is a high level semantical
information to be interpreted by the user.

>), and that there is no good way to cope with nested queries.

> A possible compromise is to put the text of the generated SQL command
> into a new field only if the error is a syntax error, and put it into
> the CONTEXT stack otherwise. Syntax errors couldn't be nested so at,
> least that problem goes away. This seems a bit messy though.

I don't like it, because above comments.

> The other thing to think about is whether we should invent a new field
> to carry syntax error position for generated queries, rather than making
> 'P' do double duty as it does now. If we don't do that then we have to
> change the protocol specification to reflect reality. In any case I
> think it has to be possible to tell very easily from the error message
> whether the 'P' position refers to the submitted query or a generated
> query.

I think a new field is alas necessary. I thought about a

P 22 SELECT something FRM table

but psql simply uses the raw "P" field content in the message, so you
would get:

... at or near character 22 SELECT something FRM table

with old clients:-(

--
Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr