Re: plpgsql.print_strict_params

Hi

Sorry for the delay on following up on this.

2013/9/18 Marko Tiikkaja <marko@joh.to>:
> Hi,
>
> Attached is a patch with the following changes:
>
> On 16/09/2013 10:57, I wrote:
>>
>> On 9/16/13 8:04 AM, Ian Lawrence Barwick wrote:
>>>
>>> However the sample function provided in the documentation throws a
>>> runtime error due to a missing FROM-clause entry.
>>
>> Ugh.  I'll look into fixing that.
>
> Fixed.

Confirmed :)

>>> 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)");
>>
>>
>> Nice catch.  Will look into this.  Another option would be to simply
>> omit the DETAIL line if there were no parameters.  At this very moment
>> I'm thinking that might be a bit nicer.
>
> Decided to remove the DETAIL line in these cases.

Yes, on reflection I think that makes most sense.

>>> 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.
>>
>> Hmm..  I should probably look at what happens when the parameters to a
>> prepared statement are currently logged and imitate that.
>
> Yup, they're escaped.  Did just that.  Also copied the "parameters: " prefix
> on the DETAIL line from there.

The output looks like this now:
 postgres=# select get_userid(E'foo'''); ERROR:  query returned more than one row DETAIL:  parameters: $1 = 'foo'''
CONTEXT: PL/pgSQL function get_userid(text) line 6 at SQL statement
 

which looks fine to me.

>>> 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?
>>
>> Agreed, they could use some renaming.
>
> I went with format_expr_params and format_preparedparamsdata.

Thanks. "format_preparedparamsdata" is a bit hard to scan, but that's
just nitpicking on my part ;)

>>> * Are the comments sufficient and accurate?
>>> "exec_get_query_params()" and "exec_get_dynquery_params()"
>>> could do with some brief comments explaining their purpose.
>>
>> Agreed.
>
> Still agreeing with both of us, added a comment each.

Thanks.

> I also added the new keywords to the unreserved_keywords production, as per
> the instructions near the beginning of pl_gram.y.

Good catch.

The patch looks good to me now; does the status need to be changed to
"Ready for Committer"?

Regards


Ian Barwick