Обсуждение: Re: [COMMITTERS] pgsql: Adjust OLDSERXID_MAX_PAGE based on BLCKSZ.
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 -Kevin
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. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > On 08.07.2011 15:22, Kevin Grittner wrote: >> 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. Good point. The old calculation was finding the page before the page which would contain the first out-of-bound entry. As long as these numbers are all powers of 2 that's the same, but (besides the overflow issue) it's not as clear. On the off chance that this saves someone any work, trivial patch attached. -Kevin
Вложения
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?
regards, tom lane
On 08.07.2011 16:45, Kevin Grittner wrote: > Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> wrote: >> On 08.07.2011 15:22, Kevin Grittner wrote: > >>> 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. > > Good point. The old calculation was finding the page before the > page which would contain the first out-of-bound entry. As long as > these numbers are all powers of 2 that's the same, but (besides the > overflow issue) it's not as clear. > > On the off chance that this saves someone any work, trivial patch > attached. There was still one warning left after that: .\src\backend\storage\lmgr\predicate.c(770): warning C4146: unary minus operator applied to unsigned type, result still unsigned It's coming from this line: > else if (diff < -((OLDSERXID_MAX_PAGE + 1) / 2)) I fixed that by adding a cast to int there, and committed. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
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
Tom Lane <tgl@sss.pgh.pa.us> wrote: > So, what are the consequences if a compiler allows the expression > to overflow to zero? Does this mean that beta3 is dangerously > broken? The risk to anyone not using serializable transactions is most definitely zero. I've been running with this patch in my daily tests (including the isolation tests and `make installcheck-world` with default_transaction_isolation = 'serializable' without seeing any problems. The best way to check would be to run the isolation tests on a platform reporting the overflow. -Kevin
On Fri, Jul 8, 2011 at 10:57 AM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > 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. Should we, then, consider rewrapping beta3? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jul 8, 2011 at 10:57 AM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> 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.
> Should we, then, consider rewrapping beta3?
At this point I think the actual choice we'd have is to abandon beta3
and try again next week with a beta4. I'm trying to figure out whether
this bug is serious enough to warrant that, but it's not clear to me.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Fri, Jul 8, 2011 at 10:57 AM, Heikki Linnakangas >> <heikki.linnakangas@enterprisedb.com> wrote: >>> 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. > >> Should we, then, consider rewrapping beta3? > > At this point I think the actual choice we'd have is to abandon > beta3 and try again next week with a beta4. I'm trying to figure > out whether this bug is serious enough to warrant that, but it's > not clear to me. I changed the definition to this: #define OLDSERXID_MAX_PAGE (-1) That caused my compiler to report the following warnings: predicate.c: In function *OldSerXidAdd*: predicate.c:828: warning: division by zero predicate.c:848: warning: division by zero predicate.c: In function *OldSerXidGetMinConflictCommitSeqNo*: predicate.c:958: warning: division by zero predicate.c: In function *CheckPointPredicate*: predicate.c:1038: warning: division by zero It's hard to imagine that any compiler would evaluate it to -1 instead of the value it's had all along (including beta2) and not generate these warnings, too. -Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> At this point I think the actual choice we'd have is to abandon
>> beta3 and try again next week with a beta4. I'm trying to figure
>> out whether this bug is serious enough to warrant that, but it's
>> not clear to me.
> I changed the definition to this:
> #define OLDSERXID_MAX_PAGE (-1)
> That caused my compiler to report the following warnings:
> predicate.c: In function *OldSerXidAdd*:
> predicate.c:828: warning: division by zero
> predicate.c:848: warning: division by zero
> predicate.c: In function *OldSerXidGetMinConflictCommitSeqNo*:
> predicate.c:958: warning: division by zero
> predicate.c: In function *CheckPointPredicate*:
> predicate.c:1038: warning: division by zero
> It's hard to imagine that any compiler would evaluate it to -1
> instead of the value it's had all along (including beta2) and not
> generate these warnings, too.
The value of OLDSERXID_ENTRIESPERPAGE includes a sizeof() call, so
every compiler I've ever heard of is going to consider it an unsigned
value, so we should be getting an unsigned comparison in the Min().
So I think you are right that OLDSERXID_MAX_PAGE should end up with
its old value. Still, it's a bit nervous-making to have such problems
popping up with a patch that went in at the eleventh hour --- and it
was about the least of the last-minute patches for SSI, too. So
color me uncomfortable ...
regards, tom lane
On Fri, Jul 8, 2011 at 11:57 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: >> Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> At this point I think the actual choice we'd have is to abandon >>> beta3 and try again next week with a beta4. I'm trying to figure >>> out whether this bug is serious enough to warrant that, but it's >>> not clear to me. > >> I changed the definition to this: > >> #define OLDSERXID_MAX_PAGE (-1) > >> That caused my compiler to report the following warnings: > >> predicate.c: In function *OldSerXidAdd*: >> predicate.c:828: warning: division by zero >> predicate.c:848: warning: division by zero >> predicate.c: In function *OldSerXidGetMinConflictCommitSeqNo*: >> predicate.c:958: warning: division by zero >> predicate.c: In function *CheckPointPredicate*: >> predicate.c:1038: warning: division by zero > >> It's hard to imagine that any compiler would evaluate it to -1 >> instead of the value it's had all along (including beta2) and not >> generate these warnings, too. > > The value of OLDSERXID_ENTRIESPERPAGE includes a sizeof() call, so > every compiler I've ever heard of is going to consider it an unsigned > value, so we should be getting an unsigned comparison in the Min(). > So I think you are right that OLDSERXID_MAX_PAGE should end up with > its old value. Still, it's a bit nervous-making to have such problems > popping up with a patch that went in at the eleventh hour --- and it > was about the least of the last-minute patches for SSI, too. So > color me uncomfortable ... I agree. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company