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 overflowThe 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
Attachments:
ssi-maxpage-rounding.patchtext/plain; name=ssi-maxpage-rounding.patchDownload
*** a/src/backend/storage/lmgr/predicate.c
--- b/src/backend/storage/lmgr/predicate.c
***************
*** 311,317 **** static SlruCtlData OldSerXidSlruCtlData;
* transactions and the maximum that SLRU supports.
*/
#define OLDSERXID_MAX_PAGE Min(SLRU_PAGES_PER_SEGMENT * 0x10000 - 1, \
! (MaxTransactionId + 1) / OLDSERXID_ENTRIESPERPAGE - 1)
#define OldSerXidNextPage(page) (((page) >= OLDSERXID_MAX_PAGE) ? 0 : (page) + 1)
--- 311,317 ----
* transactions and the maximum that SLRU supports.
*/
#define OLDSERXID_MAX_PAGE Min(SLRU_PAGES_PER_SEGMENT * 0x10000 - 1, \
! MaxTransactionId / OLDSERXID_ENTRIESPERPAGE)
#define OldSerXidNextPage(page) (((page) >= OLDSERXID_MAX_PAGE) ? 0 : (page) + 1)
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 overflowThe 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 zeroIt'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