From: | Ian Lawrence Barwick <barwick(at)gmail(dot)com> |
---|---|
To: | Marko Tiikkaja <marko(at)joh(dot)to> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: plpgsql.print_strict_params |
Date: | 2013-09-16 06:04:17 |
Message-ID: | CAB8KJ=j3PfLw11W9=36Pv7K=XWyejzJVyMXGdX5M2mfLU0UyMg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
2013/9/15 Marko Tiikkaja <marko(at)joh(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2013-09-16 06:20:44 | Re: \h open |
Previous Message | David Fetter | 2013-09-16 05:37:10 | Re: Hstore: Query speedups with Gin index |