Re: plpgsql.print_strict_params

2013/9/15 Marko Tiikkaja <marko@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