hint infrastructure setup (v3)

Fabien COELHO wrote:

Dear patchers,

Well, as the discussion rages over my previous patch submissions, I've
time to improve the code;-)

I finally figured out that there is 2 errhint functions (elog.c vs
ipc_text.c), and the one I'm calling is the fist one, so I need to put a
format. The second appears to ignore it's arguments after the first.

Anyway, please consider the following patch for inclusion to current
head. It validates for me. I need it to be able to go on.

Why did all the tags have to be renamed:

+ cmdGRANT: GRANT {noH;};

Also, what is typical output for a hint? Can you show one?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Dear Bruce,

Why did all the tags have to be renamed:

+ cmdGRANT: GRANT {noH;};

that's a good question.

In order to add hints, I want to attach them to the states of the parser
automaton. So as to do that, I need I put a name on every state, so I need
to refactor the prefix that lead to a state, at give it a name.

Otherwise, I would have :

xxx: GRANT {noH;} ALL
| GRANT {noH;} CREATE
;

(1) I would have to put the "after GRANT" hint twice, one after each
GRANT occurence.

(2) this would result in shift/reduce conflicts, as the parser does not
know which hint should be put. Well, it is the same code in both case,
but yacc does not look at that.

Also, I need to stop hints, otherwise the last 'pushed' hint would be
shown on any error later.

Also, what is typical output for a hint? Can you show one?

Well, the current status of the infrastructure is that there is no hint;-)
The only hint with the patch is if you do not put an SQL command.

It may look like :

psql> creat table foo();
ERROR: syntax error at or near "creat" at character 1
creat table foo();
^
HINT: sql command such as SELECT or CREATE... ?

All other syntax errors result in no hint being displayed, thanks to
all added noH calls I put.

Also, As far as I remember, I put a new option to activate these hints.
Hints are disactivated by default. This is because some people do not
like advices and may want to turn them on or off.

--
Fabien Coelho - coelho@cri.ensmp.fr

Fabien COELHO <coelho@cri.ensmp.fr> writes:

Also, what is typical output for a hint? Can you show one?

Well, the current status of the infrastructure is that there is no hint;-)

Ah, now I remember why I was waiting to review that stuff: I was expecting
you to come out with a version that actually did some things :-(

What you've got at the moment is a patch that significantly uglifies the
grammar without actually doing anything useful, or even clearly pointing
the way to what else will need to happen before it does do something
useful. So it's difficult to form any reasoned judgment about whether
we want to commit to following this path or not.

As I said back at the beginning, I'm unconvinced that this path leads
to anything useful --- it seems like an extremely painful way of
accomplishing less than what a simple change to auto-show the command's
syntax summary could do. To change my mind, you've got to show me some
concrete results of hints that are more useful than "\h <command>" is.

regards, tom lane

Dear Tom,

Well, the current status of the infrastructure is that there is no hint;-)

Ah, now I remember why I was waiting to review that stuff: I was expecting
you to come out with a version that actually did some things :-(

Well, if you wait for something from me, it is better to tell me directly.
I was waiting for anything to happen to the patch (accept, discuss or
reject) before submitting anything else.

What you've got at the moment is a patch that significantly uglifies the
grammar without actually doing anything useful, or even clearly pointing
the way to what else will need to happen before it does do something
useful. So it's difficult to form any reasoned judgment about whether
we want to commit to following this path or not.

I can see your point.

The reasonnable way out of the deadlock that I can suggest is the
following:

I can resubmit a new patch that would provide the needed infrastructure
AND some hints on some commands as a proof of concept of what can be
achieved.

Then you can decide whether it is worth having this kind of thing on all
commands, or not.

If not, I won't have passed 3 week-ends in the parser instead if my
garden for nothing. If you are okay then you apply, and I'll submit some
new patches later on, one command at a time, and when I have time.

Does this sound reasonnable enough?

--
Fabien Coelho - coelho@cri.ensmp.fr

Fabien COELHO <coelho@cri.ensmp.fr> writes:

I can resubmit a new patch that would provide the needed infrastructure
AND some hints on some commands as a proof of concept of what can be
achieved.

I quite agree that you shouldn't do a complete implementation when it's
not clear if we'll accept it or not. What I'd like to see is a demo,
basically. How about one example of each of several different kinds
of hints? We need to get a feeling for what can be accomplished and how
messy the grammar would get.

BTW, it seems like you are putting in a lot of infrastructure to
recreate externally the parse-stack state that bison has internally.
Maybe an interesting alternative would be to look at touching that
state directly. I realize that bison doesn't consider that part of
their official API, but bison is a stable tool and seems unlikely to
change much in the future. We could probably get away with it...

regards, tom lane

Dear Tom,

I quite agree that you shouldn't do a complete implementation when it's
not clear if we'll accept it or not. What I'd like to see is a demo,
basically. How about one example of each of several different kinds
of hints? We need to get a feeling for what can be accomplished and how
messy the grammar would get.

I've already tried some implementation but it was not showable, and
because the infrastructure was not in place, inappropriate hints could
be shown.

It is not "that" messy. Just prefix are given a name to attach a rule
for hint updates.

BTW, it seems like you are putting in a lot of infrastructure to
recreate externally the parse-stack state that bison has internally.

Yes.

Maybe an interesting alternative would be to look at touching that
state directly. I realize that bison doesn't consider that part of
their official API, but bison is a stable tool and seems unlikely to
change much in the future. We could probably get away with it...

Interesting, however:

I would not like to break postgresql portability with other parser
generators. It would be enough to reject the patch from my point of
view;-)

Using some non standard undocumented API would not help other people who
would like to touch the parser.

It would not help me either, because I would have to learn the
undocumented API;-)

I really need to give a name to the state so as to be able to attach a
hint. I cannot set how I could guess the state number given after "GRANT"
just from the source, and without generating lots of conflicts.

So I'll do some more programming maybe over the week-end a resubmit
something soon.

--
Fabien.

Fabien COELHO wrote:

I would not like to break postgresql portability with other parser
generators. It would be enough to reject the patch from my point of
view;-)

We require bison to build, period. I am sure we use enough
bison-specific stuff now. No one has suggested another parser generator
to support either.

Using some non standard undocumented API would not help other people who
would like to touch the parser.

It would not help me either, because I would have to learn the
undocumented API;-)

I really need to give a name to the state so as to be able to attach a
hint. I cannot set how I could guess the state number given after "GRANT"
just from the source, and without generating lots of conflicts.

So I'll do some more programming maybe over the week-end a resubmit
something soon.

Please show us an example of what a typical hint would look like to the
user.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Please show us an example of what a typical hint would look like to the
user.

As an illustration, here is the kind of thing I had with an early
quick and dirty proof of concept implementation I made for myself
one month ago.

As a result of this first try, I've convinced myself that I should do
that cleanly and incrementally, one command at a time, with a proper clean
infrastructure, and intermediate validations...

My idea of the target audience for those hints is my students while
they're learning SQL.

--
-- test HINTs provided on parser errors.
--
-- it should:
-- (1) trigger all possible hints
-- (2) include ACTUAL syntax errors by REAL people
--
-- Note that these test assume a psql like interface which
-- can only send "lexable" and "even" commands to the backend.
-- Thus I cannot test badly closed quotes or parentheses.
--
--
-- (1) all hints
--
--
-- sql command expected

foo;
ERROR: syntax error at or near "foo" at character 1
HINT: sql command such as SELECT, CREATE, GRANT... expected

123;
ERROR: syntax error at or near "123" at character 1
HINT: sql command such as SELECT, CREATE, GRANT... expected

+;
ERROR: syntax error at or near "+" at character 1
HINT: sql command such as SELECT, CREATE, GRANT... expected

'hello world';
ERROR: syntax error at or near "'hello world'" at character 1
HINT: sql command such as SELECT, CREATE, GRANT... expected

NULL;
ERROR: syntax error at or near "NULL" at character 1
HINT: sql command such as SELECT, CREATE, GRANT... expected

COLUMN;
ERROR: syntax error at or near "COLUMN" at character 1
HINT: sql command such as SELECT, CREATE, GRANT... expected

CREAT TABLE foo(id INT4);
ERROR: syntax error at or near "CREAT" at character 1
HINT: sql command such as SELECT, CREATE, GRANT... expected

--
-- CREATE xxx

CREATE 123;
ERROR: syntax error at or near "123" at character 8
HINT: OR REPLACE, TABLE, DATABASE, USER... expected

CREATE foo;
ERROR: syntax error at or near "foo" at character 8
HINT: OR REPLACE, TABLE, DATABASE, USER... expected

CREATE + CREATE ();
ERROR: syntax error at or near "+" at character 8
HINT: OR REPLACE, TABLE, DATABASE, USER... expected

CREATE INT4;
ERROR: syntax error at or near "INT4" at character 8
HINT: OR REPLACE, TABLE, DATABASE, USER... expected

CREATE OR foo;
ERROR: syntax error at or near "foo" at character 11
HINT: REPLACE expected

--
-- DROP xxx

DROP 123;
ERROR: syntax error at or near "123" at character 6
HINT: object such as TABLE, DATABASE, USER... expected

DROP foo;
ERROR: syntax error at or near "foo" at character 6
HINT: object such as TABLE, DATABASE, USER... expected

DROP USER 123;
ERROR: syntax error at or near "123" at character 11
HINT: user id such as calvin expected

DROP SYSID 123;
ERROR: syntax error at or near "SYSID" at character 6
HINT: object such as TABLE, DATABASE, USER... expected

DROP USER SYSID 123;
ERROR: syntax error at or near "123" at character 17

DROP GROUP 321;
ERROR: syntax error at or near "321" at character 12
HINT: group id such as admin expected

DROP GROUP calvin, foo;
ERROR: syntax error at or near "," at character 18
HINT: ; expected

DROP TABLE (foo);
ERROR: syntax error at or near "(" at character 12
HINT: table name... expected

--
-- CREATE USER

CREATE USER COLUMN;
ERROR: syntax error at or near "COLUMN" at character 13
HINT: user id such as calvin expected

CREATE USER 123;
ERROR: syntax error at or near "123" at character 13
HINT: user id such as calvin expected

CREATE USER 'hello';
ERROR: syntax error at or near "'hello'" at character 13
HINT: user id such as calvin expected

CREATE USER calvin foo;
ERROR: syntax error at or near "foo" at character 20
HINT: user options or SET or RESET expected

CREATE USER calvin WITH foo;
ERROR: syntax error at or near "foo" at character 25
HINT: user options expected

CREATE USER calvin WITH CREATEDB foo;
ERROR: syntax error at or near "foo" at character 34
HINT: other user options or ';' expected

CREATE USER calvin PASSWORD 123;
ERROR: syntax error at or near "123" at character 29
HINT: password string such as 'secret' expected

CREATE USER calvin WITH PASSWORD 123;
ERROR: syntax error at or near "123" at character 34
HINT: password string such as 'secret' expected

CREATE USER calvin WITH ENCRYPTED 'pass';
ERROR: syntax error at or near "'pass'" at character 35
HINT: PASSWORD expected

CREATE USER calvin WITH UNENCRYPTED 'pass';
ERROR: syntax error at or near "'pass'" at character 37
HINT: PASSWORD expected

CREATE USER calvin WITH ENCRYPTED PASS 123;
ERROR: syntax error at or near "PASS" at character 35
HINT: PASSWORD expected

CREATE USER calvin WITH ENCRYPTED PASSWORD 123;
ERROR: syntax error at or near "123" at character 44
HINT: password string such as 'secret' expected

CREATE USER calvin WITH UNENCRYPTED PASSWORD 'secret' CREADB;
ERROR: syntax error at or near "CREADB" at character 55
HINT: other user options or ';' expected

CREATE USER calvin NOCREATEDB foo;
ERROR: syntax error at or near "foo" at character 31
HINT: other user options or ';' expected

CREATE USER calvin WITH IN foo;
ERROR: syntax error at or near "foo" at character 28
HINT: GROUP expected

CREATE USER calvin WITH IN GROUP 'foo';
ERROR: syntax error at or near "'foo'" at character 34
HINT: group id list expected

CREATE USER calvin WITH IN GROUP 123;
ERROR: syntax error at or near "123" at character 34
HINT: group id list expected

CREATE USER calvin WITH IN GROUP admin, column;
ERROR: syntax error at or near "column" at character 41
HINT: group id such as admin expected

CREATE USER calvin WITH IN GROUP admin, 123;
ERROR: syntax error at or near "123" at character 41
HINT: group id such as admin expected

CREATE USER calvin WITH IN GROUP admin, CREATEDB;
ERROR: group "admin" does not exist

CREATE USER calvin WITH IN GROUP admin CREATEDB foo;
ERROR: syntax error at or near "foo" at character 49
HINT: other user options or ';' expected

CREATE USER calvin NOCREATEUSER CREATEDB foo;
ERROR: syntax error at or near "foo" at character 42
HINT: other user options or ';' expected

CREATE USER calvin CREATEUSER foo;
ERROR: syntax error at or near "foo" at character 31
HINT: other user options or ';' expected

CREATE USER calvin SYSID foo;
ERROR: syntax error at or near "foo" at character 26
HINT: integer constant expected

CREATE USER calvin SYSID 12 foo;
ERROR: syntax error at or near "foo" at character 29
HINT: other user options or ';' expected

CREATE USER calvin VALID UNTIL 12;
ERROR: syntax error at or near "12" at character 32
HINT: date string such as '01-02-2000' expected

CREATE USER calvin VALID foo;
ERROR: syntax error at or near "foo" at character 26
HINT: UNTIL expected

--
-- ALTER USER

ALTER USER calvin foo;
ERROR: syntax error at or near "foo" at character 19
HINT: user options or SET or RESET or ; expected

ALTER USER calvin WITH foo;
ERROR: syntax error at or near "foo" at character 24
HINT: user options expected

ALTER USER calvin SET foo;
ERROR: syntax error at or near ";" at character 26
HINT: SET stuff expected

ALTER USER calvin RESET 123;
ERROR: syntax error at or near "123" at character 25
HINT: user id such as calvin expected

--
-- CREATE GROUP

CREATE GROUP 123;
ERROR: syntax error at or near "123" at character 14
HINT: group id such as admin expected

CREATE GROUP grp foo;
ERROR: syntax error at or near "foo" at character 18
HINT: user list such as "USER calvin" or "SYSID 123" expected

CREATE GROUP grp WITH foo;
ERROR: syntax error at or near "foo" at character 23
HINT: user list such as "USER calvin" or "SYSID 123" expected

CREATE GROUP grp WITH USER ;
ERROR: syntax error at or near ";" at character 28
HINT: user id such as calvin expected

CREATE GROUP grp WITH USER 123;
ERROR: syntax error at or near "123" at character 28
HINT: user id such as calvin expected

CREATE GROUP grp WITH USER calvin, 123;
ERROR: syntax error at or near "123" at character 36
HINT: user id such as calvin expected

CREATE GROUP grp WITH SYSID calvin;
ERROR: syntax error at or near "calvin" at character 29
HINT: integer constant expected

CREATE GROUP grp WITH SYSID 'hello';
ERROR: syntax error at or near "'hello'" at character 29
HINT: integer constant expected

CREATE GROUP grp WITH USER calvin, hobbes SYSID 'hello';
ERROR: syntax error at or near "'hello'" at character 49
HINT: integer constant expected

CREATE GROUP grtp USER 123;
ERROR: syntax error at or near "123" at character 24
HINT: user id such as calvin expected

CREATE GROUP grtp SYSID calvin;
ERROR: syntax error at or near "calvin" at character 25
HINT: integer constant expected

CREATE GROUP grp WITH USER calvin, hobbes SYSID 123 foo;
ERROR: syntax error at or near "foo" at character 53
HINT: user list such as "USER calvin" or "SYSID 123" expected

--
-- ALTER GROUP

ALTER GROUP grp;
ERROR: syntax error at or near ";" at character 16
HINT: ADD or DROP expected

ALTER GROUP grp foo;
ERROR: syntax error at or near "foo" at character 17
HINT: ADD or DROP expected

ALTER GROUP grp ADD calvin;
ERROR: syntax error at or near "calvin" at character 21
HINT: USER user id list expected

ALTER GROUP grp DROP calvin;
ERROR: syntax error at or near "calvin" at character 22
HINT: USER user id list expected

ALTER GROUP grp ADD USER 123;
ERROR: syntax error at or near "123" at character 26
HINT: user id such as calvin expected

ALTER GROUP grp DROP USER 123;
ERROR: syntax error at or near "123" at character 27
HINT: user id such as calvin expected

ALTER GROUP grp ADD USER calvin, USER 123;
ERROR: syntax error at or near "USER" at character 34
HINT: user id such as calvin expected

ALTER GROUP grp DROP USER calvin, 123;
ERROR: syntax error at or near "123" at character 35
HINT: user id such as calvin expected

ALTER GROUP grp ADD SYSID calvin;
ERROR: syntax error at or near "SYSID" at character 21
HINT: USER user id list expected

ALTER GROUP grp DROP SYSID calvin;
ERROR: syntax error at or near "SYSID" at character 22
HINT: USER user id list expected

--
-- CREATE SCHEMA...

CREATE SCHEMA 123;
ERROR: syntax error at or near "123" at character 15
HINT: schema name or AUTHORIZATION expected

CREATE SCHEMA sch foo;
ERROR: syntax error at or near "foo" at character 19
HINT: AUTHORIZATION or ... expected

CREATE SCHEMA sch AUTHORIZATION 123;
ERROR: syntax error at or near "123" at character 33
HINT: user id such as calvin expected

CREATE SCHEMA AUTHORIZATION 123;
ERROR: syntax error at or near "123" at character 29
HINT: user id such as calvin expected

CREATE SCHEMA sch AUTHORIZATION calvin CREATE foo;
ERROR: syntax error at or near "foo" at character 47
HINT: TABLE, INDEX, SEQUENCE, TRIGGER or VIEW expected

CREATE SCHEMA sch AUTHORIZATION calvin CREATE TABLE +;
ERROR: syntax error at or near "+" at character 53
HINT: table name... expected

CREATE SCHEMA sch AUTHORIZATION calvin CREATE TABLE foo() xxx;
ERROR: syntax error at or near "xxx" at character 59
HINT: schema statement list such as CREATE TABLE/INDEX/SEQUENCE/TRIGGER/VIEW or GRANT or ; expected

CREATE SCHEMA sch AUTHORIZATION calvin CREATE TABLE foo() CREATE TABLE bla() x;
ERROR: syntax error at or near "x" at character 78
HINT: schema statement list such as CREATE TABLE/INDEX/SEQUENCE/TRIGGER/VIEW or GRANT or ; expected

--
Fabien Coelho - coelho@cri.ensmp.fr

Fabien COELHO <coelho@cri.ensmp.fr> writes:

I would not like to break postgresql portability with other parser
generators.

I agree with Bruce's opinion that this is a nonissue. In particular I'd
point out that the current grammar is already too big for any known yacc
clone besides bison (even with bison you need a very recent version),
and your proposed changes are going to make the grammar a whole lot
larger yet, at least in terms of states and actions. Even if there was
any usefulness to supporting other yacc clones, the odds of making one
work seem minuscule.

Using some non standard undocumented API would not help other people who
would like to touch the parser.

It would help them a *lot* if it meant that they didn't have to be aware
of this stuff in order to do anything at all with the grammar. My
fundamental objection to this proposal continues to be that it will make
the grammar unreadable and unmaintainable. If we could push the error
message generation issues into a subroutine of yyerror() and not touch
the grammar rules at all, the chances of getting this accepted would
rise about a thousand percent.

The sample hints you show in another message still aren't impressing me
much, but in any case they look like they only depend on being able to
see what grammar symbols can follow the current point. The last time
I studied this stuff (which was quite a while back) the follow set was
something an LR parser generator would have to compute anyway. I don't
know whether bison's internal tables expose that set in any useful
fashion, but it surely seems worth a look.

regards, tom lane

PS: another reason for doing it that way is that I suspect your current
approach is a dead end anyhow. You've already hit one blocker problem
where you got reduce/reduce errors when you tried to insert
state-recording actions, and you've only gone as far as hinting the very
first symbol, which is hardly one of the more complex parts of the
grammar. There are large parts of the grammar that only manage to avoid
ambiguity by the skin of their teeth --- I don't believe you'll be able
to do anything of this sort in those areas without breaking it. Take a
look at SelectStmt and subsidiary productions as an example ... that's
a fragile bit of work that took a long time to get right.

Dear Tom,

I would not like to break postgresql portability with other parser
generators.

I agree with Bruce's opinion that this is a nonissue. In particular I'd
point out that the current grammar is already too big for any known yacc
clone besides bison (even with bison you need a very recent version),
and your proposed changes are going to make the grammar a whole lot
larger yet, at least in terms of states and actions.

Not really in terms of state. The state should basically be the same.
However yes in terms of "explicit" state that are given explicit names.
And definitely in terms of actions, as you say.

Using some non standard undocumented API would not help other people who
would like to touch the parser.

It would help them a *lot* if it meant that they didn't have to be aware
of this stuff in order to do anything at all with the grammar. My
fundamental objection to this proposal continues to be that it will make
the grammar unreadable and unmaintainable. If we could push the error
message generation issues into a subroutine of yyerror() and not touch
the grammar rules at all, the chances of getting this accepted would
rise about a thousand percent.

Hmmm. I'm so much below zero! So maybe I think I won't resubmit a
"infrastructure patch with some hints for some commands", as it seems
just a lose of time.

The sample hints you show in another message still aren't impressing me
much,

I don't mean them to be impressive! It is not an expert system or a seer
I'm planing. I just want them to be useful to my students!

but in any case they look like they only depend on being able to
see what grammar symbols can follow the current point.

Yes. That's basically where the parser is when it generates an error, it
is expecting something and cannot find it. I could do more, but with more
efforts.

The last time I studied this stuff (which was quite a while back) the
follow set was something an LR parser generator would have to compute
anyway.

Yes.

I don't know whether bison's internal tables expose that set in any
useful fashion, but it surely seems worth a look.

Having a look is something I can do;-)

I'm afraid it looks like "internal state 1232, 43425 and 42523", but there
may be some support enough in the generated code to get something more
interesting. It would require to be able to get textual meta informations
out of the state number, which is possible if bison/flex people did
something about it.

I can remembre something like that, somewhere deep in my memory...

PS: another reason for doing it that way is that I suspect your current
approach is a dead end anyhow. You've already hit one blocker problem
where you got reduce/reduce errors when you tried to insert
state-recording actions, and you've only gone as far as hinting the very
first symbol, which is hardly one of the more complex parts of the
grammar.

These states exists anyway. Once the first keyword(s) are passed, there is
much less troubles as the SQL syntax is really verbose, so it is usually
hard to get lost. The only real problem I had passed the first symbol, for
the part I played with, is with SET [SESSION|LOCAL] SESSION AUTHORIZATION
which needed a real bit of refactoring.

Also, it is better to avoid some factorizations if the semantics
is different. For instance, user_list is used both for groups and users,
so I switched during my tests to a user_list and a group_list.

There are large parts of the grammar that only manage to avoid
ambiguity by the skin of their teeth --- I don't believe you'll be able
to do anything of this sort in those areas without breaking it. Take a
look at SelectStmt and subsidiary productions as an example ... that's
a fragile bit of work that took a long time to get right.

I noticed that great care has been taken to avoid any ambiguity.
I have no intention to change that.

--
Fabien Coelho - coelho@cri.ensmp.fr

Fabien COELHO wrote:

The last time I studied this stuff (which was quite a while back) the
follow set was something an LR parser generator would have to compute
anyway.

Yes.

I don't know whether bison's internal tables expose that set in any
useful fashion, but it surely seems worth a look.

Having a look is something I can do;-)

I'm afraid it looks like "internal state 1232, 43425 and 42523", but there
may be some support enough in the generated code to get something more
interesting. It would require to be able to get textual meta informations
out of the state number, which is possible if bison/flex people did
something about it.

You *really* don't want to go there. If you want to see what the parser
is doing you can run "bison -r all" over the grammar and examine the
.output file. But please, let's not examine the internal states. Talk
about unmaintainability! Also, I suspect that bison does a good bit of
optimisation by way of combining states that removes some of the
information you might need, but I haven't looked into it closely.

If we really wanted to go down this route, a predictive (i.e. LL(n)
type) parser rather than a bottom up parser would probably be more
suitable, but I doubt anyone has any stomach for doing that work, even
if there was agreement on the need for it.

cheers

andrew

Fabien COELHO <coelho@cri.ensmp.fr> writes:

... your proposed changes are going to make the grammar a whole lot
larger yet, at least in terms of states and actions.

Not really in terms of state. The state should basically be the same.
However yes in terms of "explicit" state that are given explicit names.
And definitely in terms of actions, as you say.

But mid-rule actions are implemented by inventing additional internal
productions (see the bison manual's discussion of them; the O'Reilly
lex & yacc book says that all yaccs do it like that). That's not only
more states, but more symbols, which is going to impose an O(N^2) cost
on the raw tabular representation of the parsing rules. Maybe much of
this will be bought back when bison compresses the tables, and maybe
not. Have you checked how much the size of gram.o grows with the stuff
you've installed so far?

(I'm also slightly worried about what this will do to parsing speed,
but evidence about that would have to wait for further development of
the patch. In any case it'd be more attractive if the cost of this
could be paid only when we've already detected an error...)

I'm afraid it looks like "internal state 1232, 43425 and 42523", but there
may be some support enough in the generated code to get something more
interesting. It would require to be able to get textual meta informations
out of the state number, which is possible if bison/flex people did
something about it.

The string names of the grammar symbols are all embedded in gram.c
anyway, so if you can figure out the symbols that are expected next,
their names could be printed directly. We could alter the symbol names
to be more useful to novices, or alternatively install an additional
lookup table to substitute reader-friendly phrases.

regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:

You *really* don't want to go there. If you want to see what the parser
is doing you can run "bison -r all" over the grammar and examine the
.output file. But please, let's not examine the internal states. Talk
about unmaintainability!

What I was suggesting was that we might be able to extract the "follow
set" from bison's tables, ie, the set of grammar symbols that are legal
next inputs given the current parse state stack. I surely agree that
we don't want code that goes like "if state is N then print message X"
... but the follow set should be stable. One way of describing Fabien's
existing patch is that it's essentially keeping track of the follow set
by hand :-(

Also, I suspect that bison does a good bit of
optimisation by way of combining states that removes some of the
information you might need, but I haven't looked into it closely.

That could be a showstopper if true, but it's all speculation at this
point.

regards, tom lane

Fabien COELHO wrote:

CREATE USER calvin WITH IN GROUP admin, CREATEDB;
ERROR: group "admin" does not exist

CREATE USER calvin WITH IN GROUP admin CREATEDB foo;
ERROR: syntax error at or near "foo" at character 49
HINT: other user options or ';' expected

CREATE USER calvin NOCREATEUSER CREATEDB foo;
ERROR: syntax error at or near "foo" at character 42
HINT: other user options or ';' expected

CREATE USER calvin CREATEUSER foo;
ERROR: syntax error at or near "foo" at character 31
HINT: other user options or ';' expected

I hate to say it but I don't see these hints as being very helpful.

Basically, if you look at the developers FAQ, you should discuss what
you want to do, how you want to do it, then code. I realize you spent a
lot of time on a patch, but I don't see a lot of value in adding such
hints.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Fri, 2 Apr 2004, Bruce Momjian wrote:

Fabien COELHO wrote:

CREATE USER calvin WITH IN GROUP admin, CREATEDB;
ERROR: group "admin" does not exist

CREATE USER calvin WITH IN GROUP admin CREATEDB foo;
ERROR: syntax error at or near "foo" at character 49
HINT: other user options or ';' expected

CREATE USER calvin NOCREATEUSER CREATEDB foo;
ERROR: syntax error at or near "foo" at character 42
HINT: other user options or ';' expected

CREATE USER calvin CREATEUSER foo;
ERROR: syntax error at or near "foo" at character 31
HINT: other user options or ';' expected

I hate to say it but I don't see these hints as being very helpful.

I can see them as potentially being useful for people who don't have alot
of knowledge of SQL or our dialect thereof. I think some of the ones
shown may need better wording (for example the ones above basically seem
to mean go look at the help for create user which is pretty much the same
as the error on its own, but perhaps with a longer hint, it might be less
so) but I think it illustrates the point for these kinds of queries. I'm
not sure that the HINT strings would be as meaningful in the middle of
complicated select/update/etc queries, but that would be something to see.

I'm not sure it's PostgreSQL's responsibility to teach SQL or even really
to teach our own commands, but if it were possible to do without much of a
performance, readability or maintenance cost, it'd probably be worth
doing. I can't really say much specifically about the patch either way on
any of those grounds (having not actually looked).

Stephan Szabo wrote:

I hate to say it but I don't see these hints as being very helpful.

I can see them as potentially being useful for people who don't have alot
of knowledge of SQL or our dialect thereof. I think some of the ones
shown may need better wording (for example the ones above basically seem
to mean go look at the help for create user which is pretty much the same
as the error on its own, but perhaps with a longer hint, it might be less
so) but I think it illustrates the point for these kinds of queries. I'm
not sure that the HINT strings would be as meaningful in the middle of
complicated select/update/etc queries, but that would be something to see.

I'm not sure it's PostgreSQL's responsibility to teach SQL or even really
to teach our own commands, but if it were possible to do without much of a
performance, readability or maintenance cost, it'd probably be worth
doing. I can't really say much specifically about the patch either way on
any of those grounds (having not actually looked).

I wonder if we could just do a \h command as a hint. In some cases, it
might be clearer than printing some text.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Stephan Szabo <sszabo@megazone.bigpanda.com> writes:

I'm not sure that the HINT strings would be as meaningful in the middle of
complicated select/update/etc queries, but that would be something to see.

That's pretty much my stumbling block too. The examples Fabien has
shown so far don't seem to me to be all that much more helpful than a
simple facility to show the "\h command" syntax summary. Now "\h"
doesn't know anything about syntax errors within expressions, and so
I'm prepared to believe that there's some gold to be mined here if we
can get the parser to offer useful hints in the context of complex
expressions. What I've not seen yet is any proof that that can be done
at all, much less at a reasonable cost in terms of grammar complexity...

I'm not sure it's PostgreSQL's responsibility to teach SQL or even really
to teach our own commands, but if it were possible to do without much of a
performance, readability or maintenance cost, it'd probably be worth
doing.

Right; we have to consider those costs and trade them off against the
benefit of better error reporting.

One thought here is that the extra costs might be better spent in a
client program that's in a "training wheels" mode. Would it be
reasonable to invent a "psql-student" client that has its own version of
the parser with better error detection? Then production clients
wouldn't have to pay any cycles towards support of novices. I'm not
sure right now how hard it would be to keep such a client in sync with
the production backend, but it's an idea to consider...

regards, tom lane

Dear Stephan,

I can see them as potentially being useful for people who don't have alot
of knowledge of SQL or our dialect thereof.

That is my audience.

I think some of the ones shown may need better wording [...]

Sure. I'm not the man for writing very clear English sentences.
I may be more fluent in C;-)

I'm not sure it's PostgreSQL's responsibility to teach SQL or even really
to teach our own commands,

I don't know about postgreSQL responsability, but I am a teacher using
postgreSQL as a tool for teaching database. I don't need hints, but my
students may find sql a little bit easier to grasp if they have a little
bit more help from the error reports.

I can understand that this is not the main agenda of postgresql, which
is more about new features, better performance and sustained stability.
But it is my agenda as a teacher.

--
Fabien Coelho - coelho@cri.ensmp.fr

I wonder if we could just do a \h command as a hint. In some cases, it
might be clearer than printing some text.

\h SELECT or \h CREATE TABLE are 2 pages long.

For the beginner, it is a lot of help... a little bit too much.

So I think that a better job can be done. If I'm wrong, you don't apply
a patch and that's all! Basically, this is the current status;-)

--
Fabien Coelho - coelho@cri.ensmp.fr