Обсуждение: Review: plpgsql.extra_warnings, plpgsql.extra_errors
Hello
This patch introduce a possibility to implement some new checks without impact to current code.Pavel Stehule
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 pkPavel
2014-03-19 11:28 GMT+01:00 Petr Jelinek <petr@2ndquadrant.com>:
Thanks! Attached new version of the patch with the above change.
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
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
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
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
Pavel
--
Álvaro Herrera http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
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
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
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
2014-03-20 0:32 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
>> I think this should have the GUC_LIST_INPUT flag, and ensure that whenTBH, if I thought this specific warning was the only one that would ever
>> 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.
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
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
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
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
> + myextra = (int *) malloc(sizeof(int)); Please consider not casting malloc(). See http://c-faq.com/malloc/mallocnocast.html
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
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.
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
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
Hello2014-03-20 12:39 GMT+01:00 Petr Jelinek <petr@2ndquadrant.com>:
On 20/03/14 00:32, Tom Lane wrote:Of course, nobody assumes that it will be the only one.
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.Actually it does not since it all has to be handled in check/assign hook anyway.
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.
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
2014-03-23 15:14 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:
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?The code is good - and I don't see any problem there.Tom's objections was related to GUC part. It is redesigned as Tom proposed.2. v6 patch: patching cleanly, compilation without errors and warnings, all regress tests passed1. There is a wide agreement on implemented feature - nothing changed from previous review - it is not necessary comment it again.I did a recheck a newest version of this patch:Review shadow_v6 patchHello
A using DefElem will be longer, but it is typical pattern for this case in Postgres.
What is opinion of other hackers?
Pavel
PavelRegards2014-03-20 12:39 GMT+01:00 Petr Jelinek <petr@2ndquadrant.com>:On 20/03/14 00:32, Tom Lane wrote:Of course, nobody assumes that it will be the only one.
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.Actually it does not since it all has to be handled in check/assign hook anyway.
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.
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
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
2014-03-23 15:53 GMT+01:00 Petr Jelinek <petr@2ndquadrant.com>:
SET plpgsql.extra_warnings TO 'shadowed_variables';
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.
I don't afraid so 32 bits it is too low - in this case, list can be used without compatibility issues.
if others has no problem with it, I have not a problem too.
doc should be enhanced by:SET plpgsql.extra_warnings TO 'shadowed_variables';
CREATE FUNCTION foo(f1 int) RETURNS int AS $$
DECLARE
f1 int;
BEGIN
RETURN f1;
END
$$ LANGUAGE plpgsql;
DECLARE
f1 int;
BEGIN
RETURN f1;
END
$$ LANGUAGE plpgsql;
Regards
Pavel
--
On 23/03/14 19:38, Pavel Stehule wrote: > > > doc should be enhanced by: > > <snip> > Docs updated. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
On 24 March 2014 10:58, Petr Jelinek <petr@2ndquadrant.com> wrote: > Docs updated. OK, it looks to me that all outstanding comments have been resolved. I'll be looking to commit this later today, so last call for comments. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services