From: | Petr Jelinek <petr(at)2ndquadrant(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
Cc: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Marko Tiikkaja <marko(at)joh(dot)to> |
Subject: | Re: Review: plpgsql.extra_warnings, plpgsql.extra_errors |
Date: | 2014-03-19 20:16:42 |
Message-ID: | 5329FB2A.1090503@2ndquadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2014-03-19 20:22:39 | Re: Patch to send transaction commit/rollback stats to the stats collector unconditionally. |
Previous Message | Andrew Dunstan | 2014-03-19 20:13:43 | Re: jsonb status |