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 - coelho(at)cri(dot)ensmp(dot)fr

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
> 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>

I have applied this patch, after some considerable whacking around to
make it ready to cope with multicolumn Kanji characters. It does not
actually cope yet, since the necessary knowledge is not available
from the character encoding logic. But replacing the two places
that say

scroffset += 1; /* XXX fix me when we have screen width info */

with calls to a get-the-screen-width-of-this-character subroutine should
do the job.

I have temporarily fixed the problem shown in Fabien's original
regression tests:

CREATE FUNCTION test1 (int) RETURNS int LANGUAGE sql
AS 'not even SQL';
ERROR: syntax error at or near "not" at character 1
+ LINE 1: CREATE FUNCTION test1 (int) RETURNS int LANGUAGE sql
+ ^

by having the backend output a CONTEXT field in this case, and causing
psql to ignore the cursor position if there's a CONTEXT field. This
is necessary AFAICS in the general case where a pre-existing function
gets an internal syntax error when called. It'd be nice to do better in
the case of CREATE FUNCTION for a SQL function, but I don't currently
see how we can account for the string literal's starting position and
possible internal quotes, backslashes, etc etc.

regards, tom lane

> I have applied this patch,

Thanks!

> after some considerable whacking around to make it ready to cope with
> multicolumn Kanji characters. It does not actually cope yet, since the
> necessary knowledge is not available from the character encoding logic.
> But replacing the two places that say
>
> scroffset += 1; /* XXX fix me when we have screen width info */
>
> with calls to a get-the-screen-width-of-this-character subroutine should
> do the job.

I also looked into it, and it is also a little bit more complex, as the
extract width must be though from the terminal perspective. Thus the
truncation part must take into account the terminal lengths. I think it
would require an additionnal array to store character to terminal column
mapping that would be used when truncating. I'll do that as soon as the
needed routines are there for terminal lengths, and submit a new patch.

> I have temporarily fixed the problem shown in Fabien's original
> regression tests:
>
> CREATE FUNCTION test1 (int) RETURNS int LANGUAGE sql
> AS 'not even SQL';
> ERROR: syntax error at or near "not" at character 1
> + LINE 1: CREATE FUNCTION test1 (int) RETURNS int LANGUAGE sql
> + ^
>
> [...]
>
> but I don't currently see how we can account for the string literal's
> starting position and possible internal quotes, backslashes, etc etc.

Yep. A solution would be to generate the extract from the backend, where
the information is really available, but this has been ruled out by
previous discussions...

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

Dear Tom,

> > scroffset += 1; /* XXX fix me when we have screen width info */
> >
> > with calls to a get-the-screen-width-of-this-character subroutine should
> > do the job.
>
> I also looked into it, and it is also a little bit more complex, as the
> extract width must be though from the terminal perspective. Thus the
> truncation part must take into account the terminal lengths. I think it
> would require an additionnal array to store character to terminal column
> mapping that would be used when truncating. I'll do that as soon as the
> needed routines are there for terminal lengths, and submit a new patch.

Well, I finally noticed that you did all that already;-) So I won't
have anything to do. Good;-)

I should think and check before writing...

Also you improved/simplified the code for the better.

I've seen the gettext stuff and buffer data. I was wondering how the
localisation magic could work, it is quite simpler this way. Also you
changed the palloc/pfree stuff to pg_malloc/free. I'll have to check pg
memory management...

Thanks again, have a nice day,

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

> > > scroffset += 1; /* XXX fix me when we have screen width info */
> > >
> > > with calls to a get-the-screen-width-of-this-character subroutine should
> > > do the job.

I have committed changes adding "PQdsplen()" which should do the job
above. Quick test with EUC-JP indicates that it is working, but still
need testing for many other encodings. Also, currently for UTF-8 and
MULE_INTERNAL it returns always 1, which is not correct of course. I
will look into this within a week..(i have a dealine for a magazine
article and have no time to do next 3 days)
--
Tatsuo Ishii

> > I also looked into it, and it is also a little bit more complex, as the
> > extract width must be though from the terminal perspective. Thus the
> > truncation part must take into account the terminal lengths. I think it
> > would require an additionnal array to store character to terminal column
> > mapping that would be used when truncating. I'll do that as soon as the
> > needed routines are there for terminal lengths, and submit a new patch.
>
> Well, I finally noticed that you did all that already;-) So I won't
> have anything to do. Good;-)
>
> I should think and check before writing...
>
> Also you improved/simplified the code for the better.
>
> I've seen the gettext stuff and buffer data. I was wondering how the
> localisation magic could work, it is quite simpler this way. Also you
> changed the palloc/pfree stuff to pg_malloc/free. I'll have to check pg
> memory management...
>
> Thanks again, have a nice day,
>
> --
> Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr
>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings
>