Thread: Review: plpgsql.extra_warnings, plpgsql.extra_errors

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

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@2ndquadrant.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

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@joh.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@piotr-stefaniak.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@piotr-stefaniak.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.

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

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@2ndquadrant.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