Re: [HACKERS] cleanup in code
On Wed, Jan 8, 2014 at 1:25 AM, Heikki Linnakangas wrote: > On 01/07/2014 05:20 PM, Tom Lane wrote: >> >> David Rowley writes: >>> >>> I think it will be like Andres said up thread, to stop multiple >>> evaluations >>> of the expression passed to the macro. >> >> >> Exactly. We are not going to risk multiple evals in a macro as commonly >> used as elog/ereport; the risk/benefit ratio is just too high. >> >> I don't see anything wrong with suppressing this warning by inserting >> an additional return statement. The code is already plastered with such >> things, from the days before we had any unreachability hints in >> elog/ereport. And as I said upthread, there is no good reason to suppose >> that the unreachability hints are always recognized by every compiler. >> I take this behavior of MSVC as proof of that statement. > > > Yeah, I was just surprised because I thought MSVC understood it. Committed > the additional return statement. Thanks for committing both the patches for cleanup. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- 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] cleanup in code
On 01/07/2014 05:20 PM, Tom Lane wrote: David Rowley writes: I think it will be like Andres said up thread, to stop multiple evaluations of the expression passed to the macro. Exactly. We are not going to risk multiple evals in a macro as commonly used as elog/ereport; the risk/benefit ratio is just too high. I don't see anything wrong with suppressing this warning by inserting an additional return statement. The code is already plastered with such things, from the days before we had any unreachability hints in elog/ereport. And as I said upthread, there is no good reason to suppose that the unreachability hints are always recognized by every compiler. I take this behavior of MSVC as proof of that statement. Yeah, I was just surprised because I thought MSVC understood it. Committed the additional return statement. - Heikki -- 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] cleanup in code
David Rowley writes: > I think it will be like Andres said up thread, to stop multiple evaluations > of the expression passed to the macro. Exactly. We are not going to risk multiple evals in a macro as commonly used as elog/ereport; the risk/benefit ratio is just too high. I don't see anything wrong with suppressing this warning by inserting an additional return statement. The code is already plastered with such things, from the days before we had any unreachability hints in elog/ereport. And as I said upthread, there is no good reason to suppose that the unreachability hints are always recognized by every compiler. I take this behavior of MSVC as proof of that statement. It is mildly curious that MSVC fails to understand the unreachability hint here when it does so elsewhere, but for our purposes, that's a purely academic question. 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
Re: [HACKERS] cleanup in code
On Tue, Jan 7, 2014 at 7:46 PM, Amit Kapila wrote: > Having said above, I think there must be a reason to have elevel_ which I > am > not aware. > > I think it will be like Andres said up thread, to stop multiple evaluations of the expression passed to the macro. If someone did: elog(mylevel++, "Something bad just happened"); With assigning this to a variable the user will have the mylevel increase by 1, but if we didn't then it would get increase more times. I didn't look at all the elog usages in core, but my guess is that we're probably not using any expressions like this in elog, but we can't really speak for any third party code which uses the macro. Likely to get rid of that variable something would have to go into the release notes to get users to check for volatile expressions being used in elog and fix them up. The only other way to fix it that I can think of is the patch you posted above which is pretty much the same one as I posted on the other thread too. Regards David Rowley
Re: [HACKERS] cleanup in code
On Mon, Jan 6, 2014 at 4:08 PM, Heikki Linnakangas
wrote:
> On 01/04/2014 07:20 AM, Amit Kapila wrote:
>>
>> 1. compiling with msvc shows warning in relcache.c
>>
>> 1>e:\workspace\postgresql\master\postgresql\src\backend\utils\cache\relcache.c(3959):
>> warning C4715: 'RelationGetIndexAttrBitmap' : not all control paths
>> return a value
>>
>> Attached patch remove_msvc_warning.patch to remove above warning
>
>
> Hmm, I thought we gave enough hints in the elog macro to tell the compiler
> that elog(ERROR) does no return, since commit
> b853eb97182079dcd30b4f52576bd5d6c275ee71. Have we not enabled that for MSVC?
I think it is enabled as part of commit
52906f175a05a4e7e5e4a0e6021c32b1bfb221cf.
The reason why we are seeing this warning is it doesn't reach pg_unreachable due
to if loop. I think the reason mention by David is right.
However if I just change the code of elog to not use elevel_ variable
it works fine.
Changing code like below removes warning and passes all regression on windows.
#define elog(elevel, ...) \
do { \
elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
elog_finish(elevel, __VA_ARGS__); \
if (elevel >= ERROR) \
pg_unreachable(); \
} while(0)
Having said above, I think there must be a reason to have elevel_ which I am
not aware.
> Do you see the warning both with asserts enabled and non-assert builds?
Yes.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
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] cleanup in code
On Tue, Jan 7, 2014 at 12:06 AM, Andres Freund wrote: > On 2014-01-06 23:51:52 +1300, David Rowley wrote: > > I looked at this a while back here: > > > http://www.postgresql.org/message-id/caaphdvqosb4nc3og0xoboj2fma-6akihuwsad43rlekqk6s...@mail.gmail.com > > > > And found that because elevel was being assigned to a variable that the > > compiler could not determine that the if (elevel_ >= ERROR) was constant > > therefore couldn't assume that __assume(0) would be reached with the > > microsoft compiler > > But afair the declaration for elog() works in several other places, so > that doesn't sufficiently explain this. I'd very much expect that that > variable is complitely elided by any halfway competent compiler - it's > just there to prevent multiple evaluation should elevel not be a > constant. > Just to add more proof to my theory; If I do this: //#define pg_unreachable() __assume(0) #define pg_unreachable() (void)0 I get no extra warnings. If change the elog macro to get rid of the variable so that the if condition uses the constant then the postgres.exe goes from 4,545,024 bytes to 4,526,592 bytes. So I guess the __assume(0) does not do much due to elevel being assigned to the variable in the elog macro. Regards David Rowley > Do you see the warning both with asserts enabled and non-assert builds? > > Greetings, > > Andres Freund > > -- > Andres Freund http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services >
Re: [HACKERS] cleanup in code
On 2014-01-06 09:54:15 -0500, Tom Lane wrote: > Andres Freund writes: > >> On Mon, Jan 6, 2014 at 11:38 PM, Heikki Linnakangas > >> wrote: > >>> Hmm, I thought we gave enough hints in the elog macro to tell the compiler > >>> that elog(ERROR) does no return, since commit > >>> b853eb97182079dcd30b4f52576bd5d6c275ee71. > > > But afair the declaration for elog() works in several other places, so > > that doesn't sufficiently explain this. I'd very much expect that that > > variable is complitely elided by any halfway competent compiler - it's > > just there to prevent multiple evaluation should elevel not be a > > constant. > > At -O0 (or local equivalent), it would not surprise me at all that > compilers wouldn't recognize elog(ERROR) as not returning. You have a point, but I don't think that any of the compilers we try to keep clean have such behaviour in the common case - at least most versions of gcc certainly recognize such on -O0, and I am pretty sure that 52906f175a05a4e7e5e4a0e6021c32b1bfb221cf fixed some warnings for mvcc, at least in some versions and some configurations. So I am wondering if there's a special reason it doesn't recognize this individual case as it does seem to work in others - defining pg_unreachable() to be empty generates about a dozen warnings here. Greetings, Andres Freund -- Andres Freund 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
Re: [HACKERS] cleanup in code
Andres Freund writes: >> On Mon, Jan 6, 2014 at 11:38 PM, Heikki Linnakangas >> wrote: >>> Hmm, I thought we gave enough hints in the elog macro to tell the compiler >>> that elog(ERROR) does no return, since commit >>> b853eb97182079dcd30b4f52576bd5d6c275ee71. > But afair the declaration for elog() works in several other places, so > that doesn't sufficiently explain this. I'd very much expect that that > variable is complitely elided by any halfway competent compiler - it's > just there to prevent multiple evaluation should elevel not be a > constant. At -O0 (or local equivalent), it would not surprise me at all that compilers wouldn't recognize elog(ERROR) as not returning. I don't think there's ever been any expectation that that marking would be bulletproof. We added it to permit better code generation, not to silence reachability warnings. 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
Re: [HACKERS] cleanup in code
On 2014-01-06 23:51:52 +1300, David Rowley wrote: > On Mon, Jan 6, 2014 at 11:38 PM, Heikki Linnakangas > wrote: > > > On 01/04/2014 07:20 AM, Amit Kapila wrote: > > > >> 1. compiling with msvc shows warning in relcache.c > >> 1>e:\workspace\postgresql\master\postgresql\src\backend\ > >> utils\cache\relcache.c(3959): > >> warning C4715: 'RelationGetIndexAttrBitmap' : not all control paths > >> return a value > >> > >> Attached patch remove_msvc_warning.patch to remove above warning > >> > > > > Hmm, I thought we gave enough hints in the elog macro to tell the compiler > > that elog(ERROR) does no return, since commit > > b853eb97182079dcd30b4f52576bd5d6c275ee71. > > Have we not enabled that for MSVC? > > > > > I looked at this a while back here: > http://www.postgresql.org/message-id/caaphdvqosb4nc3og0xoboj2fma-6akihuwsad43rlekqk6s...@mail.gmail.com > > And found that because elevel was being assigned to a variable that the > compiler could not determine that the if (elevel_ >= ERROR) was constant > therefore couldn't assume that __assume(0) would be reached with the > microsoft compiler But afair the declaration for elog() works in several other places, so that doesn't sufficiently explain this. I'd very much expect that that variable is complitely elided by any halfway competent compiler - it's just there to prevent multiple evaluation should elevel not be a constant. Do you see the warning both with asserts enabled and non-assert builds? Greetings, Andres Freund -- Andres Freund 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
Re: [HACKERS] cleanup in code
On Mon, Jan 6, 2014 at 11:38 PM, Heikki Linnakangas wrote: > On 01/04/2014 07:20 AM, Amit Kapila wrote: > >> 1. compiling with msvc shows warning in relcache.c >> 1>e:\workspace\postgresql\master\postgresql\src\backend\ >> utils\cache\relcache.c(3959): >> warning C4715: 'RelationGetIndexAttrBitmap' : not all control paths >> return a value >> >> Attached patch remove_msvc_warning.patch to remove above warning >> > > Hmm, I thought we gave enough hints in the elog macro to tell the compiler > that elog(ERROR) does no return, since commit > b853eb97182079dcd30b4f52576bd5d6c275ee71. > Have we not enabled that for MSVC? > > I looked at this a while back here: http://www.postgresql.org/message-id/caaphdvqosb4nc3og0xoboj2fma-6akihuwsad43rlekqk6s...@mail.gmail.com And found that because elevel was being assigned to a variable that the compiler could not determine that the if (elevel_ >= ERROR) was constant therefore couldn't assume that __assume(0) would be reached with the microsoft compiler Regards David Rowley
Re: [HACKERS] cleanup in code
On 01/04/2014 07:20 AM, Amit Kapila wrote: 1. compiling with msvc shows warning in relcache.c 1>e:\workspace\postgresql\master\postgresql\src\backend\utils\cache\relcache.c(3959): warning C4715: 'RelationGetIndexAttrBitmap' : not all control paths return a value Attached patch remove_msvc_warning.patch to remove above warning Hmm, I thought we gave enough hints in the elog macro to tell the compiler that elog(ERROR) does no return, since commit b853eb97182079dcd30b4f52576bd5d6c275ee71. Have we not enabled that for MSVC? 2. It seems option K is not used in pg_dump: while ((c = getopt_long(argc, argv, "abcCd:E:f:F:h:ij:K:n:N:oOp:RsS:t:T:U:vwWxZ:", long_options, &optindex)) != -1) I have checked both docs and code but didn't find the use of this option. Am I missing something here? Attached patch remove_redundant_option_K_pgdump.patch to remove this option from code. Huh. That was added by my commit that added --dbname option, by accident. Removed, thanks. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
