Hello Tom,
Yeah, I doubt that this technique for getting the raw locations in the grammar+lexer works reliably.
It worked reliably on all tests, and the assumption seemed sound to me, given the one-look-ahead property and that statement reductions can only triggered when ';' or end of input comes.
Anyway, I decided to see what it would take to do it the way I had in mind, which was to stick a separate RawStmt node atop each statement parsetree. The project turned out a bit larger than I hoped :-(,
Indeed: I tried it as it looked elegant, but it did not show to be the simple thing you announced...
[...] Furthermore, the output of pg_plan_queries is now always a list of PlannedStmt nodes, even for utility statements.
I stopped exactly there when I tried: I thought that changing that would be enough to get a reject because it would be considered much too invasive in too many places for fixing a small bug.
So this ends up costing one extra palloc per statement, or two extra in the case of a utility statement, but really that's quite negligible compared to everything else that happens in processing a statement.
As against that, I think it makes the related code a lot clearer.
Having spent some time there, I agree that a refactoring and cleanup was somehow needed...
The sheer size of the patch is a bit more than Fabien's patch,
Yep, nearly twice as big and significantly more complex, but the result is a definite improvement.
but what it is touching is not per-statement-type code but code that works generically with lists of statements. So I think it's less likely to cause problems for other patches.
Patch applies (with "patch", "git apply" did not like it though), compiles, overall make check ok, pgss make check ok as well. I do not know whether the coverage of pg tests is enough to know whether anything is broken...
I noticed that you also improved the pgss fix and tests. Good. I was unsure about removing spaces at the end of the query because of possible encoding issues.
The update end trick is nice to deal with empty statements. I wonder whether the sub "query" in Copy, View, CreateAs, Explain, Prepare, Execute, Declare... could be typed RawStmt* instead of Node*. That would avoid some casts in updateRawStmtEnd, but maybe add some other elsewhere.
One point bothered me a bit when looking at the initial code, that your refactoring has not changed: the location is about a string which is not accessible from the node, so that the string must be kept elsewhere and passed as a second argument here and there. ISTM that it would make some sense to have the initial string directly available from RawStmt, Query and PlannedStmt.
-- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
