Re: plpgsql.print_strict_params

Hi,

Attached is a patch for optionally printing more information on STRICT
failures in PL/PgSQL:

set plpgsql.print_strict_params to true;

create or replace function footest() returns void as $$
declare
x record;
p1 int := 2;
p3 text := 'foo';
begin
-- too many rows
select * from foo where f1 > p1 or f1::text = p3 into strict x;
raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
end$$ language plpgsql;
select footest();
ERROR: query returned more than one row
DETAIL: p1 = '2', p3 = 'foo'
CONTEXT: PL/pgSQL function footest() line 8 at SQL statement

This parameter is turned off by default to preserve old behaviour, but
can be extremely useful when debugging code in test environments.

I will add this to the open commitfest, but in the meanwhile, any
feedback is appreciated.

Regards,
Marko Tiikkaja

Marko Tiikkaja wrote:

> set plpgsql.print_strict_params to true;

Hmm, shouldn't this be a compile option? See the "comp_option"
production in pl_gram.y for existing examples.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-09-14 00:39, Alvaro Herrera wrote:
>> set plpgsql.print_strict_params to true;
>
> Hmm, shouldn't this be a compile option? See the "comp_option"
> production in pl_gram.y for existing examples.

I thought about that, but it seemed more like a runtime option to me.
Any particular reason you think it would be better as a compile time option?

Regards,
Marko Tiikkaja

Marko Tiikkaja wrote:
> On 2013-09-14 00:39, Alvaro Herrera wrote:
> >>set plpgsql.print_strict_params to true;
> >
> >Hmm, shouldn't this be a compile option? See the "comp_option"
> >production in pl_gram.y for existing examples.
>
> I thought about that, but it seemed more like a runtime option to
> me. Any particular reason you think it would be better as a compile
> time option?

Seems like something that would be useful to change per function, rather
than globally, no?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-09-14 03:33, Alvaro Herrera wrote:
> Marko Tiikkaja wrote:
>> I thought about that, but it seemed more like a runtime option to
>> me. Any particular reason you think it would be better as a compile
>> time option?
>
> Seems like something that would be useful to change per function, rather
> than globally, no?

The way I see it, STRICT is like an assertion, and you *would* pretty
much always change it globally.

However, it would be useful to also have the option to set it for a
single function only. Will look into that. Thanks for the suggestion!

Regards,
Marko Tiikkaja

On Fri, 2013-09-13 at 23:56 +0200, Marko Tiikkaja wrote:
> Attached is a patch for optionally printing more information on STRICT
> failures in PL/PgSQL:

Please fix the tabs in the SGML files.

On 15/09/2013 04:02, Peter Eisentraut wrote:
> On Fri, 2013-09-13 at 23:56 +0200, Marko Tiikkaja wrote:
>> Attached is a patch for optionally printing more information on STRICT
>> failures in PL/PgSQL:
>
> Please fix the tabs in the SGML files.

Oops. Thanks.

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.

Regards,
Marko Tiikkaja

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

On 9/16/13 8:04 AM, Ian Lawrence Barwick wrote:
> I'm taking a look at this patch as part of the current commitfest [*],

Thanks!

> 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.

> * 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.

Agreed. I have two patches like this on the commitfest and one more
cooking, so if more than one of these make it into PG, we should
probably discuss how the general mechanism should work and look like in
the future.

> * 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)");

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.

> 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.

> * 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?

Agreed, they could use some renaming.

> * 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.

> (It's the first time I've "formally" reviewed a patch for a commitfest
> so please let me know if I'm missing something.)

I personally think you did an excellent job. Huge thanks so far!

Regards,
Marko Tiikkaja

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.

>> 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.

>> 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 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.

>> * 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.

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

Regards,
Marko Tiikkaja

Hi

Sorry for the delay on following up on this.

2013/9/18 Marko Tiikkaja <marko(at)joh(dot)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

On 2013-09-28 12:31, Ian Lawrence Barwick wrote:
> The patch looks good to me now; does the status need to be changed to
> "Ready for Committer"?

Yes.

Thanks for reviewing!

Regards,
Marko Tiikkaja

On Sat, Sep 28, 2013 at 8:42 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> On 2013-09-28 12:31, Ian Lawrence Barwick wrote:
>> The patch looks good to me now; does the status need to be changed to
>> "Ready for Committer"?
>
> Yes.
>
> Thanks for reviewing!

This looks like a nice clean patch. My only concern is that it makes
"on" and "off" unreserved plpgsql keywords. It looks like that will
make them unusable as unquoted identifiers in a few contexts in which
they can now be used. Has there been any discussion about whether
that's OK?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 10/3/13 6:55 PM, Robert Haas wrote:
> This looks like a nice clean patch. My only concern is that it makes
> "on" and "off" unreserved plpgsql keywords. It looks like that will
> make them unusable as unquoted identifiers in a few contexts in which
> they can now be used. Has there been any discussion about whether
> that's OK?

I don't think there has.

I originally had more ideas for options which you could turn on/off,
which I believe might have justified reserving them, but I'm not sure
any of those will ever happen, at least not as a simple on/off option.
Did you have a better idea for the syntax? The only thing I can think
of is print_strict_params and no_print_strict_params, and I'm not very
fond of that.

Also, in what contexts are unreserved keywords a problem in modern PL/PgSQL?

Regards,
Marko Tiikkaja

On Fri, Oct 4, 2013 at 3:53 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> I don't think there has.
>
> I originally had more ideas for options which you could turn on/off, which I
> believe might have justified reserving them, but I'm not sure any of those
> will ever happen, at least not as a simple on/off option. Did you have a
> better idea for the syntax? The only thing I can think of is
> print_strict_params and no_print_strict_params, and I'm not very fond of
> that.

Yeah, that doesn't seem good. How about writing the grammar production as

'#' K_PRINT_STRICT_PARAMS option_value

where option_value := T_WORD | unreserved_keyword;

Then you don't need to make ON and OFF keywords; you can just use
strcmp() against the option_value and either throw an error, or set
the flag appropriately.

> Also, in what contexts are unreserved keywords a problem in modern PL/PgSQL?

I am not sure I've found all cases where this can matter, but what I
did is flipped through the grammar in less, looking for T_WORD, and
checking for productions where T_WORD was allowed but
unreserved_keyword was not. I found getdiag_target, for_variable,
stmt_execsql, and cursor_variable.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 04/10/2013 16:08, Robert Haas wrote:
> Yeah, that doesn't seem good. How about writing the grammar production as
>
> '#' K_PRINT_STRICT_PARAMS option_value
>
> where option_value := T_WORD | unreserved_keyword;
>
> Then you don't need to make ON and OFF keywords; you can just use
> strcmp() against the option_value and either throw an error, or set
> the flag appropriately.

Something like the attached?

Regards,
Marko Tiikkaja

On Sat, Oct 5, 2013 at 6:15 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> Something like the attached?

Looks good to me. Committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 10/7/13 9:46 PM, Robert Haas wrote:
> Looks good to me. Committed.

Thanks.

Also huge thanks to Ian for a phenomenal first review. :-)

Regards,
Marko Tiikkaja