Review: plpgsql.extra_warnings, plpgsql.extra_errors

On 19/03/14 09:45, Pavel Stehule wrote:

Hello

This patch introduce a possibility to implement some new checks without
impact to current code.

1. there is a common agreement about this functionality, syntax, naming

2. patching is clean, compilation is without error and warning

3. all regress tests passed

4. feature is well documented

5. code is clean, documented and respect out codding standards

Note: please, replace "shadowed-variables" by "shadowed_variables"

This patch is ready for commit

Thanks! Attached new version of the patch with the above change.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hello

all is pk

Pavel

2014-03-19 11:28 GMT+01:00 Petr Jelinek <petr@2ndquadrant.com>:

Why start a new thread for this review? It seems to me it'd be more
comfortable to keep the review as a reply on the original thread.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-03-19 13:51 GMT+01:00 Alvaro Herrera <alvherre@2ndquadrant.com>:

Why start a new thread for this review? It seems to me it'd be more
comfortable to keep the review as a reply on the original thread.

I am sorry, I though so review should to start in separate thread

Pavel

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Petr Jelinek <petr@2ndquadrant.com> writes:

On 19/03/14 19:26, Alvaro Herrera wrote:

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.

TBH, if I thought this specific warning was the only one that would ever
be there, I'd probably be arguing to reject this patch altogether.
Isn't the entire point to create a framework in which more tests will
be added later?

Also, adding GUC_LIST_INPUT later is not really cool since it changes
the parsing behavior for the GUC. If it's going to be a list, it should
be one from day zero.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-03-20 0:32 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:

Petr Jelinek <petr@2ndquadrant.com> writes:

On 19/03/14 19:26, Alvaro Herrera wrote:

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.

TBH, if I thought this specific warning was the only one that would ever
be there, I'd probably be arguing to reject this patch altogether.
Isn't the entire point to create a framework in which more tests will
be added later?

I plan to work on plpgsql redesign this summer, so plpgsql check with same
functionality can be on next release, but should not be too.

This functionality doesn't change anything - and when we will have a better
tools, we can replace it without any cost, so I am for integration. It can
helps - plpgsql_check is next level, but it is next level complexity and
now it is not simply to integrate it. Proposed feature can server lot of
users and it is good API when we integrate some more sophisticated tool. I
like this interface - it is simple and good for almost all use cases that I
can to see.

Regards

Pavel

On 3/20/14, 12:32 AM, Tom Lane wrote:

Isn't the entire point to create a framework in which more tests will
be added later?

Also, adding GUC_LIST_INPUT later is not really cool since it changes
the parsing behavior for the GUC. If it's going to be a list, it should
be one from day zero.

I'm not sure what exactly you mean by this. If the only allowed values
are "none", "variable_shadowing" and "all", how is the behaviour for
those going to change if we make it a list for 9.5?

Regards,
Marko Tiikkaja

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 20/03/14 00:32, Tom Lane wrote:

TBH, if I thought this specific warning was the only one that would ever
be there, I'd probably be arguing to reject this patch altogether.

Of course, nobody assumes that it will be the only one.

Also, adding GUC_LIST_INPUT later is not really cool since it changes
the parsing behavior for the GUC. If it's going to be a list, it should
be one from day zero.

Actually it does not since it all has to be handled in check/assign hook
anyway.

But nevertheless, I made V6 with doc change suggested by Alvaro and also
added this list handling framework for the GUC params.
In the end it is probably less confusing now that the implementation
uses bitmask instead of bool when the user facing functionality talks
about list...

This obviously needs code review again (I haven't changed tests since
nothing changed from user perspective).

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Marko Tiikkaja <marko@joh.to> writes:

On 3/20/14, 12:32 AM, Tom Lane wrote:

Also, adding GUC_LIST_INPUT later is not really cool since it changes
the parsing behavior for the GUC. If it's going to be a list, it should
be one from day zero.

I'm not sure what exactly you mean by this. If the only allowed values
are "none", "variable_shadowing" and "all", how is the behaviour for
those going to change if we make it a list for 9.5?

If we switch to using SplitIdentifierString later, which is the typical
implementation of parsing list GUCs, that will do things like case-fold,
remove double quotes, remove white space. It's possible that that's
completely upward-compatible with what happens if you don't do that ...
but I'm not sure about it.

In any case, if the point of this patch is to provide a framework for
extra error detection, I'm not sure why we'd arbitrarily say we're going
to leave the framework unfinished in the GUC department.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

+ myextra = (int *) malloc(sizeof(int));

Please consider not casting malloc(). See
http://c-faq.com/malloc/mallocnocast.html

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:

+ myextra = (int *) malloc(sizeof(int));

Please consider not casting malloc(). See

That code is per project style, and should stay that way.

http://c-faq.com/malloc/mallocnocast.html

That argument is entirely bogus, as it considers only one possible way
in which the call could be wrong; a way that is of very low probability
in PG usage, since we include <stdlib.h> in our core headers. Besides
which, as noted in the page itself, most modern compilers would warn
anyway if you forgot the inclusion.

On the other side, coding with the explicit cast helps guard against far
more dangerous coding errors, which the compiler will *not* help you with.
What if myextra is actually of type "int64 *"? In that case you probably
meant to make enough space for an int64 not an int. But without the cast,
you won't be told you did anything wrong. This is a particular hazard if
you change your mind later on about the type of myextra. (A colleague
at Salesforce got burnt in exactly that way, just a couple days ago.)

So, general policy around here is that malloc and palloc calls should look
like

ptr = (foo *) malloc(n * sizeof(foo));

so that there's a direct, visible connection between the size calculation
and the type of the resulting pointer.

I'm aware that there are some places in the code that fail to do this,
but they are not models to emulate.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03/22/2014 04:00 PM, Tom Lane wrote:

That argument is entirely bogus, as it considers only one possible way
in which the call could be wrong; a way that is of very low probability
in PG usage, since we include <stdlib.h> in our core headers. Besides
which, as noted in the page itself, most modern compilers would warn
anyway if you forgot the inclusion.

Apart from what the page says, I also think of casting malloc() as bad
style and felt the need to bring this up. But since you pointed out why
you don't want to remove the cast, I withdraw my previous suggestion.

On the other side, coding with the explicit cast helps guard against far
more dangerous coding errors, which the compiler will *not* help you with.
What if myextra is actually of type "int64 *"? In that case you probably
meant to make enough space for an int64 not an int. But without the cast,
you won't be told you did anything wrong. This is a particular hazard if
you change your mind later on about the type of myextra. (A colleague
at Salesforce got burnt in exactly that way, just a couple days ago.)

So perhaps this alternative:
myextra = malloc(sizeof *myextra);

PS.
Coding style matters to me, but I was and still am far from insisting on
anything.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:

Apart from what the page says, I also think of casting malloc() as bad
style and felt the need to bring this up.

Well, that's a value judgement I don't happen to agree with. Yeah, it'd
be better if the language design were such that we could avoid explicit
casting everywhere, but in this context casting is less risky than not
casting.

So perhaps this alternative:
myextra = malloc(sizeof *myextra);

[ shrug... ] That's about a wash for this exact use case, but it gets
messy as soon as the lefthand side is anything more complicated than a
simple variable name. And it doesn't scale to cases where the malloc
result isn't directly assigned to anything --- for example, what if
you want to pass the result of palloc() directly to some other
function, or return it from the current function?

The bigger picture though is that the style with the explicit cast is
already extremely widely used in Postgres. That being the case,
conforming to project style is better than using some inconsistent
convention, regardless of your personal views about whether there's a
better way to do it.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03/22/2014 04:00 PM, Tom Lane wrote:

On the other side, coding with the explicit cast helps guard against far
more dangerous coding errors, which the compiler will*not* help you with.
What if myextra is actually of type "int64 *"?

Indeed, neither "gcc -Wall -Wextra -std=c89 -pedantic" nor "clang
-Weverything -Wno-shadow -std=c89 -pedantic" issues a warning in such
case. "clang --analyze", however, does. Perhaps TenDRA would, if it ever
worked.

This message is meant to be merely informative, since I've put some
effort into this test. I'm not trying to argue.

Review shadow_v6 patch

Hello

I did a recheck a newest version of this patch:

1. There is a wide agreement on implemented feature - nothing changed from
previous review - it is not necessary comment it again.

2. v6 patch: patching cleanly, compilation without errors and warnings, all
regress tests passed

Tom's objections was related to GUC part. It is redesigned as Tom proposed.

The code is good - and I don't see any problem there.

I have only one objection - What I remember - more usual is using a list
instead a bitmap for these purposes - typical is DefElem struct. Isn't it
better?

Regards

Pavel

2014-03-20 12:39 GMT+01:00 Petr Jelinek <petr@2ndquadrant.com>:

2014-03-23 15:14 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:

Review shadow_v6 patch

Hello

I did a recheck a newest version of this patch:

1. There is a wide agreement on implemented feature - nothing changed from
previous review - it is not necessary comment it again.

2. v6 patch: patching cleanly, compilation without errors and warnings,
all regress tests passed

Tom's objections was related to GUC part. It is redesigned as Tom proposed.

The code is good - and I don't see any problem there.

I have only one objection - What I remember - more usual is using a list
instead a bitmap for these purposes - typical is DefElem struct. Isn't it
better?

A using DefElem will be longer, but it is typical pattern for this case in
Postgres.

What is opinion of other hackers?

Pavel

On 23/03/14 15:14, Pavel Stehule wrote:

Review shadow_v6 patch

I have only one objection - What I remember - more usual is using a list
instead a bitmap for these purposes - typical is DefElem struct. Isn't
it better?

To me it seemed that for similar use cases (list of boolean options) the
bitmap is more common in the existing code, question might be if we go
over the 32 bits any time soon which does not seem likely to me for the
checks.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers