Обсуждение: 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