Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-24 Thread Petr Jelinek
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 diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml index

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-24 Thread Simon Riggs
On 24 March 2014 10:58, Petr Jelinek p...@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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-23 Thread Pavel Stehule
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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-23 Thread Pavel Stehule
2014-03-23 15:14 GMT+01:00 Pavel Stehule pavel.steh...@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:

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-23 Thread Petr Jelinek
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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-23 Thread Pavel Stehule
2014-03-23 15:53 GMT+01:00 Petr Jelinek p...@2ndquadrant.com: 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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-22 Thread Piotr Stefaniak
+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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-22 Thread Tom Lane
Piotr Stefaniak postg...@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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-22 Thread Piotr Stefaniak
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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-22 Thread Tom Lane
Piotr Stefaniak postg...@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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-22 Thread Piotr Stefaniak
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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-20 Thread Marko Tiikkaja
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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-20 Thread Petr Jelinek
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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-20 Thread Tom Lane
Marko Tiikkaja ma...@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

[HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-19 Thread Pavel Stehule
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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-19 Thread Petr Jelinek
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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-19 Thread Pavel Stehule
Hello all is pk Pavel 2014-03-19 11:28 GMT+01:00 Petr Jelinek p...@2ndquadrant.com: 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,

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-19 Thread Alvaro Herrera
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-19 Thread Pavel Stehule
2014-03-19 13:51 GMT+01:00 Alvaro Herrera alvhe...@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 -- Álvaro

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-19 Thread Alvaro Herrera
Petr Jelinek escribió: + para + These additional checks are enabled through the configuration variables + varnameplpgsql.extra_warnings/ for warnings and + varnameplpgsql.extra_errors/ for errors. Both can be set either to + a comma-separated list of checks, literalnone/ or literalall/.

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-19 Thread Petr Jelinek
On 19/03/14 19:26, Alvaro Herrera wrote: Petr Jelinek escribió: + para + These additional checks are enabled through the configuration variables + varnameplpgsql.extra_warnings/ for warnings and + varnameplpgsql.extra_errors/ for errors. Both can be set either to + a comma-separated list

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-19 Thread Tom Lane
Petr Jelinek p...@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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

2014-03-19 Thread Pavel Stehule
2014-03-20 0:32 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Petr Jelinek p...@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,