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 <coe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
