Re: Re: [COMMITTERS] pgsql: Adjust OLDSERXID_MAX_PAGE based on BLCKSZ.

On 08.07.2011 17:29, Tom Lane wrote:
> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  writes:
>> On 08.07.2011 15:22, Kevin Grittner wrote:
>>> Heikki Linnakangas  wrote:
>>>> I'm getting a bunch of warnings on Windows related to this:
>>>> .\src\backend\storage\lmgr\predicate.c(768): warning C4307: '+' :
>>>> integral constant overflow
>
>>> The part of the expression which is probably causing this:
>>>
>>> (MaxTransactionId + 1) / OLDSERXID_ENTRIESPERPAGE - 1
>>>
>>> Which I fear may not be getting into overflow which will not do the
>>> right thing even where there is no warning.  :-(
>>>
>>> Would it be safe to assume that integer division would do the right
>>> thing if we drop both of the "off by one" adjustments and use?:
>>>
>>> MaxTransactionId / OLDSERXID_ENTRIESPERPAGE
>
>> Hmm, that seems more correct to me anyway. We are trying to calculate
>> which page xid MaxTransactionId would be stored on, if the SLRU didn't
>> have a size limit. You calculate that with simply MaxTransactionId /
>> OLDSERXID_ENTRIESPERPAGE.
>
> So, what are the consequences if a compiler allows the expression to
> overflow to zero?  Does this mean that beta3 is dangerously broken?

The whole expression was this:

> /*
>  * Set maximum pages based on the lesser of the number needed to track all
>  * transactions and the maximum that SLRU supports.
>  */
> #define OLDSERXID_MAX_PAGE    Min(SLRU_PAGES_PER_SEGMENT * 0x10000 - 1, \
>                     (MaxTransactionId + 1) / OLDSERXID_ENTRIESPERPAGE - 1)

So if MaxTransactionId+1 overflows to zero, OLDSERXID_MAX_PAGE becomes 
-1. Or a very high value, if the result of that is unsigned, as at least 
MSVC seems to interpret it given the other warning I got. If it's 
interpreted as a large unsigned value, then the SLRU_PAGES_PER_SEGMENT * 
0x10000 - 1 value wins. That's what what we had prior to this patch, in 
beta2, so we're back to square one. If it's interpreted as signed -1, 
then bad things will happen as soon as the SLRU is used.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com