Re: Review: plpgsql.extra_warnings, plpgsql.extra_errors

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