Review: plpgsql.extra_warnings, plpgsql.extra_errors

Hello

This patch introduce a possibility to implement some new checks without
impact to current code.

1. there is a common agreement about this functionality, syntax, naming

2. patching is clean, compilation is without error and warning

3. all regress tests passed

4. feature is well documented

5. code is clean, documented and respect out codding standards

Note: please, replace "shadowed-variables" by "shadowed_variables"

This patch is ready for commit

Regards

Pavel Stehule

On 19/03/14 09:45, Pavel Stehule wrote:
> Hello
>
> This patch introduce a possibility to implement some new checks without
> impact to current code.
>
> 1. there is a common agreement about this functionality, syntax, naming
>
> 2. patching is clean, compilation is without error and warning
>
> 3. all regress tests passed
>
> 4. feature is well documented
>
> 5. code is clean, documented and respect out codding standards
>
>
> Note: please, replace "shadowed-variables" by "shadowed_variables"
>
> This patch is ready for commit
>

Thanks! Attached new version of the patch with the above change.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hello

all is pk

Pavel

2014-03-19 11:28 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)com>:

>
> On 19/03/14 09:45, Pavel Stehule wrote:
>
>> Hello
>>
>> This patch introduce a possibility to implement some new checks without
>> impact to current code.
>>
>> 1. there is a common agreement about this functionality, syntax, naming
>>
>> 2. patching is clean, compilation is without error and warning
>>
>> 3. all regress tests passed
>>
>> 4. feature is well documented
>>
>> 5. code is clean, documented and respect out codding standards
>>
>>
>> Note: please, replace "shadowed-variables" by "shadowed_variables"
>>
>> This patch is ready for commit
>>
>>
>
> Thanks! Attached new version of the patch with the above change.
>
>
> --
> Petr Jelinek http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

Why start a new thread for this review? It seems to me it'd be more
comfortable to keep the review as a reply on the original thread.

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

2014-03-19 13:51 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:

> Why start a new thread for this review? It seems to me it'd be more
> comfortable to keep the review as a reply on the original thread.
>

I am sorry, I though so review should to start in separate thread

Pavel

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

Petr Jelinek escribió:

> + <para>
> + These additional checks are enabled through the configuration variables
> + <varname>plpgsql.extra_warnings</> for warnings and
> + <varname>plpgsql.extra_errors</> for errors. Both can be set either to
> + a comma-separated list of checks, <literal>"none"</> or <literal>"all"</>.
> + The default is <literal>"none"</>. Currently the list of available checks
> + includes only one:
> + <variablelist>
> + <varlistentry>
> + <term><varname>shadowed_variables</varname></term>
> + <listitem>
> + <para>
> + Checks if a declaration shadows a previously defined variable. For
> + example (with <varname>plpgsql.extra_warnings</> set to
> + <varname>shadowed_variables</varname>):
> +<programlisting>
> +CREATE FUNCTION foo(f1 int) RETURNS int AS $$
> +DECLARE
> +f1 int;
> +BEGIN
> +RETURN f1;
> +END
> +$$ LANGUAGE plpgsql;
> +WARNING: variable "f1" shadows a previously defined variable
> +LINE 3: f1 int;
> + ^
> +CREATE FUNCTION
> +</programlisting>
> + </para>
> + </listitem>
> + </varlistentry>
> + </variablelist>

As a matter of style, I think the example should go after the
<variablelist> is closed. Perhaps in the future, when we invent more
extra warnings/errors, we might want to show more than one in a single
example, for compactness.

> +static bool
> +plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source)
> +{
> + if (strcmp(*newvalue, "all") == 0 ||
> + strcmp(*newvalue, "shadowed_variables") == 0 ||
> + strcmp(*newvalue, "none") == 0)
> + return true;
> + return false;
> +}

I'm not too clear on how this works when there is more than one possible
value.

> + DefineCustomStringVariable("plpgsql.extra_warnings",
> + gettext_noop("List of programming constructs which should produce a warning."),
> + NULL,
> + &plpgsql_extra_warnings_list,
> + "none",
> + PGC_USERSET, 0,
> + plpgsql_extra_checks_check_hook,
> + plpgsql_extra_warnings_assign_hook,
> + NULL);

I think this should have the GUC_LIST_INPUT flag, and ensure that when
multiple values are passed, we can process them all in a sane fashion.

Other than this, the patch looks sane to me in a quick skim.

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

On 19/03/14 19:26, Alvaro Herrera wrote:
> Petr Jelinek escribió:
>
>> + <para>
>> + These additional checks are enabled through the configuration variables
>> + <varname>plpgsql.extra_warnings</> for warnings and
>> + <varname>plpgsql.extra_errors</> for errors. Both can be set either to
>> + a comma-separated list of checks, <literal>"none"</> or <literal>"all"</>.
>> + The default is <literal>"none"</>. Currently the list of available checks
>> + includes only one:
>> + <variablelist>
>> + <varlistentry>
>> + <term><varname>shadowed_variables</varname></term>
>> + <listitem>
>> + <para>
>> + Checks if a declaration shadows a previously defined variable. For
>> + example (with <varname>plpgsql.extra_warnings</> set to
>> + <varname>shadowed_variables</varname>):
>> +<programlisting>
>> +CREATE FUNCTION foo(f1 int) RETURNS int AS $$
>> +DECLARE
>> +f1 int;
>> +BEGIN
>> +RETURN f1;
>> +END
>> +$$ LANGUAGE plpgsql;
>> +WARNING: variable "f1" shadows a previously defined variable
>> +LINE 3: f1 int;
>> + ^
>> +CREATE FUNCTION
>> +</programlisting>
>> + </para>
>> + </listitem>
>> + </varlistentry>
>> + </variablelist>
>
> As a matter of style, I think the example should go after the
> <variablelist> is closed. Perhaps in the future, when we invent more
> extra warnings/errors, we might want to show more than one in a single
> example, for compactness.

Ok I can change that.

>> +static bool
>> +plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source)
>
> I'm not too clear on how this works when there is more than one possible
> value.
>
>> + DefineCustomStringVariable("plpgsql.extra_warnings",
>> + gettext_noop("List of programming constructs which should produce a warning."),
>> + NULL,
>> + &plpgsql_extra_warnings_list,
>> + "none",
>> + PGC_USERSET, 0,
>> + plpgsql_extra_checks_check_hook,
>> + plpgsql_extra_warnings_assign_hook,
>> + NULL);
>
> I think this should have the GUC_LIST_INPUT flag, and ensure that when
> multiple values are passed, we can process them all in a sane fashion.
>

Well, as we said with Marko in the original thread, the proper handling
is left for whoever wants to add additional parameters, for the current
implementation proper list handling is not really needed and it will
only server to increase complexity of this simple patch quite late in
the release cycle.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Petr Jelinek <petr(at)2ndquadrant(dot)com> writes:
> On 19/03/14 19:26, Alvaro Herrera wrote:
>> I think this should have the GUC_LIST_INPUT flag, and ensure that when
>> multiple values are passed, we can process them all in a sane fashion.

> Well, as we said with Marko in the original thread, the proper handling
> is left for whoever wants to add additional parameters, for the current
> implementation proper list handling is not really needed and it will
> only server to increase complexity of this simple patch quite late in
> the release cycle.

TBH, if I thought this specific warning was the only one that would ever
be there, I'd probably be arguing to reject this patch altogether.
Isn't the entire point to create a framework in which more tests will
be added later?

Also, adding GUC_LIST_INPUT later is not really cool since it changes
the parsing behavior for the GUC. If it's going to be a list, it should
be one from day zero.

regards, tom lane

2014-03-20 0:32 GMT+01:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Petr Jelinek <petr(at)2ndquadrant(dot)com> writes:
> > On 19/03/14 19:26, Alvaro Herrera wrote:
> >> I think this should have the GUC_LIST_INPUT flag, and ensure that when
> >> multiple values are passed, we can process them all in a sane fashion.
>
> > Well, as we said with Marko in the original thread, the proper handling
> > is left for whoever wants to add additional parameters, for the current
> > implementation proper list handling is not really needed and it will
> > only server to increase complexity of this simple patch quite late in
> > the release cycle.
>
> TBH, if I thought this specific warning was the only one that would ever
> be there, I'd probably be arguing to reject this patch altogether.
> Isn't the entire point to create a framework in which more tests will
> be added later?
>

I plan to work on plpgsql redesign this summer, so plpgsql check with same
functionality can be on next release, but should not be too.

This functionality doesn't change anything - and when we will have a better
tools, we can replace it without any cost, so I am for integration. It can
helps - plpgsql_check is next level, but it is next level complexity and
now it is not simply to integrate it. Proposed feature can server lot of
users and it is good API when we integrate some more sophisticated tool. I
like this interface - it is simple and good for almost all use cases that I
can to see.

Regards

Pavel

>
> Also, adding GUC_LIST_INPUT later is not really cool since it changes
> the parsing behavior for the GUC. If it's going to be a list, it should
> be one from day zero.
>
> regards, tom lane
>

On 3/20/14, 12:32 AM, Tom Lane wrote:
> Isn't the entire point to create a framework in which more tests will
> be added later?
>
> Also, adding GUC_LIST_INPUT later is not really cool since it changes
> the parsing behavior for the GUC. If it's going to be a list, it should
> be one from day zero.

I'm not sure what exactly you mean by this. If the only allowed values
are "none", "variable_shadowing" and "all", how is the behaviour for
those going to change if we make it a list for 9.5?

Regards,
Marko Tiikkaja

On 20/03/14 00:32, Tom Lane wrote:
>
> TBH, if I thought this specific warning was the only one that would ever
> be there, I'd probably be arguing to reject this patch altogether.

Of course, nobody assumes that it will be the only one.

>
> Also, adding GUC_LIST_INPUT later is not really cool since it changes
> the parsing behavior for the GUC. If it's going to be a list, it should
> be one from day zero.
>

Actually it does not since it all has to be handled in check/assign hook
anyway.

But nevertheless, I made V6 with doc change suggested by Alvaro and also
added this list handling framework for the GUC params.
In the end it is probably less confusing now that the implementation
uses bitmask instead of bool when the user facing functionality talks
about list...

This obviously needs code review again (I haven't changed tests since
nothing changed from user perspective).

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Marko Tiikkaja <marko(at)joh(dot)to> writes:
> On 3/20/14, 12:32 AM, Tom Lane wrote:
>> Also, adding GUC_LIST_INPUT later is not really cool since it changes
>> the parsing behavior for the GUC. If it's going to be a list, it should
>> be one from day zero.

> I'm not sure what exactly you mean by this. If the only allowed values
> are "none", "variable_shadowing" and "all", how is the behaviour for
> those going to change if we make it a list for 9.5?

If we switch to using SplitIdentifierString later, which is the typical
implementation of parsing list GUCs, that will do things like case-fold,
remove double quotes, remove white space. It's possible that that's
completely upward-compatible with what happens if you don't do that ...
but I'm not sure about it.

In any case, if the point of this patch is to provide a framework for
extra error detection, I'm not sure why we'd arbitrarily say we're going
to leave the framework unfinished in the GUC department.

regards, tom lane

> + myextra = (int *) malloc(sizeof(int));

Please consider not casting malloc(). See
http://c-faq.com/malloc/mallocnocast.html

Piotr Stefaniak <postgres(at)piotr-stefaniak(dot)me> writes:
>>> + myextra = (int *) malloc(sizeof(int));

> Please consider not casting malloc(). See

That code is per project style, and should stay that way.

> http://c-faq.com/malloc/mallocnocast.html

That argument is entirely bogus, as it considers only one possible way
in which the call could be wrong; a way that is of very low probability
in PG usage, since we include <stdlib.h> in our core headers. Besides
which, as noted in the page itself, most modern compilers would warn
anyway if you forgot the inclusion.

On the other side, coding with the explicit cast helps guard against far
more dangerous coding errors, which the compiler will *not* help you with.
What if myextra is actually of type "int64 *"? In that case you probably
meant to make enough space for an int64 not an int. But without the cast,
you won't be told you did anything wrong. This is a particular hazard if
you change your mind later on about the type of myextra. (A colleague
at Salesforce got burnt in exactly that way, just a couple days ago.)

So, general policy around here is that malloc and palloc calls should look
like

ptr = (foo *) malloc(n * sizeof(foo));

so that there's a direct, visible connection between the size calculation
and the type of the resulting pointer.

I'm aware that there are some places in the code that fail to do this,
but they are not models to emulate.

regards, tom lane

On 03/22/2014 04:00 PM, Tom Lane wrote:
> That argument is entirely bogus, as it considers only one possible way
> in which the call could be wrong; a way that is of very low probability
> in PG usage, since we include <stdlib.h> in our core headers. Besides
> which, as noted in the page itself, most modern compilers would warn
> anyway if you forgot the inclusion.
>
Apart from what the page says, I also think of casting malloc() as bad
style and felt the need to bring this up. But since you pointed out why
you don't want to remove the cast, I withdraw my previous suggestion.

> On the other side, coding with the explicit cast helps guard against far
> more dangerous coding errors, which the compiler will *not* help you with.
> What if myextra is actually of type "int64 *"? In that case you probably
> meant to make enough space for an int64 not an int. But without the cast,
> you won't be told you did anything wrong. This is a particular hazard if
> you change your mind later on about the type of myextra. (A colleague
> at Salesforce got burnt in exactly that way, just a couple days ago.)
>
So perhaps this alternative:
myextra = malloc(sizeof *myextra);

PS.
Coding style matters to me, but I was and still am far from insisting on
anything.

Piotr Stefaniak <postgres(at)piotr-stefaniak(dot)me> writes:
> Apart from what the page says, I also think of casting malloc() as bad
> style and felt the need to bring this up.

Well, that's a value judgement I don't happen to agree with. Yeah, it'd
be better if the language design were such that we could avoid explicit
casting everywhere, but in this context casting is less risky than not
casting.

> So perhaps this alternative:
> myextra = malloc(sizeof *myextra);

[ shrug... ] That's about a wash for this exact use case, but it gets
messy as soon as the lefthand side is anything more complicated than a
simple variable name. And it doesn't scale to cases where the malloc
result isn't directly assigned to anything --- for example, what if
you want to pass the result of palloc() directly to some other
function, or return it from the current function?

The bigger picture though is that the style with the explicit cast is
already extremely widely used in Postgres. That being the case,
conforming to project style is better than using some inconsistent
convention, regardless of your personal views about whether there's a
better way to do it.

regards, tom lane

On 03/22/2014 04:00 PM, Tom Lane wrote:
> On the other side, coding with the explicit cast helps guard against far
> more dangerous coding errors, which the compiler will*not* help you with.
> What if myextra is actually of type "int64 *"?
Indeed, neither "gcc -Wall -Wextra -std=c89 -pedantic" nor "clang
-Weverything -Wno-shadow -std=c89 -pedantic" issues a warning in such
case. "clang --analyze", however, does. Perhaps TenDRA would, if it ever
worked.

This message is meant to be merely informative, since I've put some
effort into this test. I'm not trying to argue.

Review shadow_v6 patch

Hello

I did a recheck a newest version of this patch:

1. There is a wide agreement on implemented feature - nothing changed from
previous review - it is not necessary comment it again.

2. v6 patch: patching cleanly, compilation without errors and warnings, all
regress tests passed

Tom's objections was related to GUC part. It is redesigned as Tom proposed.

The code is good - and I don't see any problem there.

I have only one objection - What I remember - more usual is using a list
instead a bitmap for these purposes - typical is DefElem struct. Isn't it
better?

Regards

Pavel

2014-03-20 12:39 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)com>:

> On 20/03/14 00:32, Tom Lane wrote:
>
>>
>> TBH, if I thought this specific warning was the only one that would ever
>> be there, I'd probably be arguing to reject this patch altogether.
>>
>
> Of course, nobody assumes that it will be the only one.
>
>
>
>> Also, adding GUC_LIST_INPUT later is not really cool since it changes
>> the parsing behavior for the GUC. If it's going to be a list, it should
>> be one from day zero.
>>
>>
> Actually it does not since it all has to be handled in check/assign hook
> anyway.
>
> But nevertheless, I made V6 with doc change suggested by Alvaro and also
> added this list handling framework for the GUC params.
> In the end it is probably less confusing now that the implementation uses
> bitmask instead of bool when the user facing functionality talks about
> list...
>
> This obviously needs code review again (I haven't changed tests since
> nothing changed from user perspective).
>
>
>
> --
> Petr Jelinek http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

2014-03-23 15:14 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> Review shadow_v6 patch
>
> Hello
>
> I did a recheck a newest version of this patch:
>
> 1. There is a wide agreement on implemented feature - nothing changed from
> previous review - it is not necessary comment it again.
>
> 2. v6 patch: patching cleanly, compilation without errors and warnings,
> all regress tests passed
>
> Tom's objections was related to GUC part. It is redesigned as Tom proposed.
>
> The code is good - and I don't see any problem there.
>
> I have only one objection - What I remember - more usual is using a list
> instead a bitmap for these purposes - typical is DefElem struct. Isn't it
> better?
>

A using DefElem will be longer, but it is typical pattern for this case in
Postgres.

What is opinion of other hackers?

Pavel

>
> Regards
>
> Pavel
>
>
> 2014-03-20 12:39 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)com>:
>
> On 20/03/14 00:32, Tom Lane wrote:
>>
>>>
>>> TBH, if I thought this specific warning was the only one that would ever
>>> be there, I'd probably be arguing to reject this patch altogether.
>>>
>>
>> Of course, nobody assumes that it will be the only one.
>>
>>
>>
>>> Also, adding GUC_LIST_INPUT later is not really cool since it changes
>>> the parsing behavior for the GUC. If it's going to be a list, it should
>>> be one from day zero.
>>>
>>>
>> Actually it does not since it all has to be handled in check/assign hook
>> anyway.
>>
>> But nevertheless, I made V6 with doc change suggested by Alvaro and also
>> added this list handling framework for the GUC params.
>> In the end it is probably less confusing now that the implementation uses
>> bitmask instead of bool when the user facing functionality talks about
>> list...
>>
>> This obviously needs code review again (I haven't changed tests since
>> nothing changed from user perspective).
>>
>>
>>
>> --
>> Petr Jelinek http://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Training & Services
>>
>
>

On 23/03/14 15:14, Pavel Stehule wrote:
> Review shadow_v6 patch
>
> I have only one objection - What I remember - more usual is using a list
> instead a bitmap for these purposes - typical is DefElem struct. Isn't
> it better?
>

To me it seemed that for similar use cases (list of boolean options) the
bitmap is more common in the existing code, question might be if we go
over the 32 bits any time soon which does not seem likely to me for the
checks.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2014-03-23 15:53 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)com>:

>
>
> On 23/03/14 15:14, Pavel Stehule wrote:
>
>> Review shadow_v6 patch
>>
>>
>> I have only one objection - What I remember - more usual is using a list
>> instead a bitmap for these purposes - typical is DefElem struct. Isn't
>> it better?
>>
>>
> To me it seemed that for similar use cases (list of boolean options) the
> bitmap is more common in the existing code, question might be if we go over
> the 32 bits any time soon which does not seem likely to me for the checks.
>

I don't afraid so 32 bits it is too low - in this case, list can be used
without compatibility issues.

if others has no problem with it, I have not a problem too.

doc should be enhanced by:

SET plpgsql.extra_warnings TO 'shadowed_variables';

CREATE FUNCTION foo(f1 int) RETURNS int AS $$
DECLARE
f1 int;
BEGIN
RETURN f1;
END
$$ LANGUAGE plpgsql;

Regards

Pavel

> --
> Petr Jelinek http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

On 23/03/14 19:38, Pavel Stehule wrote:
>
>
> doc should be enhanced by:
>
> <snip>
>

Docs updated.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 24 March 2014 10:58, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> Docs updated.

OK, it looks to me that all outstanding comments have been resolved.

I'll be looking to commit this later today, so last call for comments.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services