Re: Review: plpgsql.extra_warnings, plpgsql.extra_errors

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