Re: Syntax error reporting (was Re: [PATCHES] syntax error position "CREATE FUNCTION" bug fix)

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> 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.

Right, but you quite properly didn't like my quick-hack to psql that
assumes that the presence of any CONTEXT means it's not a top-level
syntax error.  I would like to replace that hack with something cleaner.

>> 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.

The parser needn't do anything different.  What I'm imagining is that
for internally submitted queries, there will always be an
error_context_stack routine that can transform the error report into
whatever we decide the appropriate format is.

>> 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.

The point is that CONTEXT is essentially a record of "how we got here".
In a situation where the actual error occurs inside a couple of levels
of nesting, you want to be able to report the outer queries as well as
the one that directly caused the error.  I agree that there's probably
little hope of clients automatically making sense of the CONTEXT info.
But we need to provide it to help people debug complex functions.

> More over, I have other ideas for CONTEXT, which should really be a stack.

It already is a stack.

>> 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.

> I think a new field is alas necessary.

I'm leaning in that direction also.  How about

'P': used only for syntax error position in the client-submitted query.

'p': syntax error position in an internally-generated query.

'q': text of an internally-generated query ('p' and 'q' would always    appear together).

In the case of a non-syntax error in an internally generated query, we
should stick the query text into the CONTEXT stack, since it might not
be the most closely nested context item anyway.

An existing client will ignore the 'p' and 'q' fields, thus providing
behavior that's no worse than what you get with 7.4 now.
        regards, tom lane