Hello,
It is big, but I think quite reasonable. The refactoring makes
many things clearer and I like it. No objection for the
patch. The affect of changing some API's are not a problem.
At Thu, 12 Jan 2017 19:00:47 +0100 (CET), Fabien COELHO <coelho@cri.ensmp.fr> wrote in
<alpine.DEB.2.20.1701121752430.3788@lancre>
> 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...
The same for me. There's some parts that I haven't fully
understand, though..
> 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.
I found an inconsistency (in style, not function:) between
copyfuncs and equalfuncs. The patch handles the three members
utilityStmt, stmt_location and stmt_len of Query at once and
copyfuncs seems to be edited to follow the rule, but in
equalfuncs the position of utilityStmt is not moved.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center