2013/9/15 Marko Tiikkaja <ma...@joh.to>: > > Attached an updated patch to fix the tabs and to change this to a > compile-time option. Now it's not possible to flexibly disable the feature > during runtime, but I think that's fine.
I'm taking a look at this patch as part of the current commitfest [*], (It's the first time I've "formally" reviewed a patch for a commitfest so please let me know if I'm missing something.) [*] https://commitfest.postgresql.org/action/patch_view?id=1221 Submission review ----------------- * Is the patch in a patch format which has context? Yes. * Does it apply cleanly to the current git master? Yes. No new files are introduced by this patch. * Does it include reasonable tests, necessary doc patches, etc? Yes. However the sample function provided in the documentation throws a runtime error due to a missing FROM-clause entry. Usability review ---------------- * Does the patch actually implement that? Yes. * Do we want that? It seems like it would be useful; no opposition has been raised on -hackers so far. * Do we already have it? No. * Does it follow SQL spec, or the community-agreed behavior? SQL spec: n/a. I do note that it makes use of the "#" syntax before the body of a PL/pgSQL function, which is currently only used for "#variable_conflict" [*]. I can imagine this syntax might be used for other purposes down the road, so it might be worth keeping an eye on it before it becomes a hodge-podge of ad-hoc options. [*] http://www.postgresql.org/docs/current/static/plpgsql-implementation.html * Does it include pg_dump support (if applicable)? n/a * Are there dangers? I don't see any. * Have all the bases been covered? This lineis in "exec_get_query_params()" and "exec_get_dynquery_params()": return "(no parameters)"; Presumably the message will escape translation and this line should be better written as: return _("(no parameters)"); Also, if the offending query parameter contains a single quote, the output is not escaped: postgres=# select get_userid(E'foo'''); ERROR: query returned more than one row DETAIL: p1 = 'foo'' CONTEXT: PL/pgSQL function get_userid(text) line 9 at SQL statement Not sure if that's a real problem though. Feature test ------------ * Does the feature work as advertised? Yes. * Are there corner cases the author has failed to consider? I can't see any. * Are there any assertion failures or crashes? No. Performance review ------------------ * Does the patch slow down simple tests? No. * If it claims to improve performance, does it? n/a * Does it slow down other things? No. Coding review ------------- * Does it follow the project coding guidelines? Yes. The functions added in pl_exec.c - "exec_get_query_params()" and "exec_get_dynquery_params()" do strike me as potentially misnamed, as they don't actually execute anything but are more utility functions for generating formatted output. Maybe "format_query_params()" etc. would be better? * Are there portability issues? I don't think so. * Will it work on Windows/BSD etc? Tested on OS X and Linux. I don't see anything, e.g. system calls, which might stop it working on Windows. * Are the comments sufficient and accurate? "exec_get_query_params()" and "exec_get_dynquery_params()" could do with some brief comments explaining their purpose. * Does it do what it says, correctly? As far as I can tell, yes. * Does it produce compiler warnings? No. * Can you make it crash? So far, no. Architecture review ------------------- * Is everything done in a way that fits together coherently with other features/modules? Patch affects PL/pgSQL only. * Are there interdependencies that can cause problems? No. Regards Ian Barwick -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
