Re: Review: plpgsql.extra_warnings, plpgsql.extra_errors - Mailing list pgsql-hackers

From Petr Jelinek
Subject Re: Review: plpgsql.extra_warnings, plpgsql.extra_errors
Date
Msg-id 5329FB2A.1090503@2ndquadrant.com
Whole thread Raw
In response to Re: Review: plpgsql.extra_warnings, plpgsql.extra_errors  (Alvaro Herrera <alvherre@2ndquadrant.com>)
Responses Re: Review: plpgsql.extra_warnings, plpgsql.extra_errors
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Andrew Dunstan
Date:
Subject: Re: jsonb status
Next
From: Tom Lane
Date:
Subject: Re: Patch to send transaction commit/rollback stats to the stats collector unconditionally.