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