Re: Small SSI issues
Heikki Linnakangas wrote:
* The oldserxid code is broken for non-default BLCKSZ.
o The warning will come either too early or too late
o There is no safeguard against actually wrapping around the
SLRU, just the warning
o I'm suspicious of the OldSerXidPagePrecedesLogically() logic
with 32k BLCKSZ. In that case, OLDSERXID_MAX_PAGE is a larger
than necessary to cover the whole range of 2^32 transactions,
so at high XIDs, say 2^32-1, doesn't it incorrectly think that
low XIDs, say 10, are in the past, not the future?
It took me a while to see these problems because somehow I had
forgotten that SLRU_PAGES_PER_SEGMENT was a literal of 32 rather than
being based on BLCKSZ. After I rediscovered that, your concern was
clear enough.
I think the attached patch addresses the problem with the
OldSerXidPagePrecedesLogically() function, which strikes me as the
most serious issue.
Based on the calculation from the attached patch, it would be easy to
adjust the warning threshold, but I'm wondering if we should just rip
it out instead. As I mentioned in a recent post based on reviewing
your concerns, with an 8KB BLCKSZ the SLRU system will start thinking
we're into wraparound at one billion transactions, and refuse to
truncate segment files until we get down to less than that, but we
won't actually overwrite anything and cause SSI misbehaviors until we
eat through two billion (2^31 really) transactions while holding open
a single read-write transaction. At that point I think PostgreSQL
has other defenses which come into play. With the attached patch I
don't think we can have any such problems with a *larger* BLCKSZ, so
the only point of the warning would be for a BLCKSZ of 4KB or less.
Is it worth carrying the warning code (with an appropriate adjustment
to the thresholds) or should we drop it?
-Kevin
Attachments:
ssi-slru-maxpage.patchapplication/octet-stream; name=ssi-slru-maxpage.patchDownload
*** a/src/backend/storage/lmgr/predicate.c
--- b/src/backend/storage/lmgr/predicate.c
***************
*** 296,302 **** static SlruCtlData OldSerXidSlruCtlData;
#define OLDSERXID_PAGESIZE BLCKSZ
#define OLDSERXID_ENTRYSIZE sizeof(SerCommitSeqNo)
#define OLDSERXID_ENTRIESPERPAGE (OLDSERXID_PAGESIZE / OLDSERXID_ENTRYSIZE)
! #define OLDSERXID_MAX_PAGE (SLRU_PAGES_PER_SEGMENT * 0x10000 - 1)
#define OldSerXidNextPage(page) (((page) >= OLDSERXID_MAX_PAGE) ? 0 : (page) + 1)
--- 296,308 ----
#define OLDSERXID_PAGESIZE BLCKSZ
#define OLDSERXID_ENTRYSIZE sizeof(SerCommitSeqNo)
#define OLDSERXID_ENTRIESPERPAGE (OLDSERXID_PAGESIZE / OLDSERXID_ENTRYSIZE)
!
! /*
! * 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)
#define OldSerXidNextPage(page) (((page) >= OLDSERXID_MAX_PAGE) ? 0 : (page) + 1)
On Sun, Jun 19, 2011 at 7:17 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
Heikki Linnakangas wrote:
* The oldserxid code is broken for non-default BLCKSZ.
o The warning will come either too early or too late
o There is no safeguard against actually wrapping around the
SLRU, just the warning
o I'm suspicious of the OldSerXidPagePrecedesLogically() logic
with 32k BLCKSZ. In that case, OLDSERXID_MAX_PAGE is a larger
than necessary to cover the whole range of 2^32 transactions,
so at high XIDs, say 2^32-1, doesn't it incorrectly think that
low XIDs, say 10, are in the past, not the future?It took me a while to see these problems because somehow I had
forgotten that SLRU_PAGES_PER_SEGMENT was a literal of 32 rather than
being based on BLCKSZ. After I rediscovered that, your concern was
clear enough.I think the attached patch addresses the problem with the
OldSerXidPagePrecedesLogically() function, which strikes me as the
most serious issue.Based on the calculation from the attached patch, it would be easy to
adjust the warning threshold, but I'm wondering if we should just rip
it out instead. As I mentioned in a recent post based on reviewing
your concerns, with an 8KB BLCKSZ the SLRU system will start thinking
we're into wraparound at one billion transactions, and refuse to
truncate segment files until we get down to less than that, but we
won't actually overwrite anything and cause SSI misbehaviors until we
eat through two billion (2^31 really) transactions while holding open
a single read-write transaction. At that point I think PostgreSQL
has other defenses which come into play. With the attached patch I
don't think we can have any such problems with a *larger* BLCKSZ, so
the only point of the warning would be for a BLCKSZ of 4KB or less.
Is it worth carrying the warning code (with an appropriate adjustment
to the thresholds) or should we drop it?
Is this still an open item?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Jun 19, 2011 at 7:17 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:Heikki Linnakangas wrote:
* The oldserxid code is broken for non-default BLCKSZ.
o The warning will come either too early or too late
o There is no safeguard against actually wrapping around the
SLRU, just the warning
o I'm suspicious of the OldSerXidPagePrecedesLogically() logic
with 32k BLCKSZ. In that case, OLDSERXID_MAX_PAGE is a larger
than necessary to cover the whole range of 2^32 transactions,
so at high XIDs, say 2^32-1, doesn't it incorrectly think that
low XIDs, say 10, are in the past, not the future?It took me a while to see these problems because somehow I had
forgotten that SLRU_PAGES_PER_SEGMENT was a literal of 32 rather
than being based on BLCKSZ. After I rediscovered that, your
concern was clear enough.I think the attached patch addresses the problem with the
OldSerXidPagePrecedesLogically() function, which strikes me as
the most serious issue.Based on the calculation from the attached patch, it would be
easy to adjust the warning threshold, but I'm wondering if we
should just rip it out instead. As I mentioned in a recent post
based on reviewing your concerns, with an 8KB BLCKSZ the SLRU
system will start thinking we're into wraparound at one billion
transactions, and refuse to truncate segment files until we get
down to less than that, but we won't actually overwrite anything
and cause SSI misbehaviors until we eat through two billion (2^31
really) transactions while holding open a single read-write
transaction. At that point I think PostgreSQL has other defenses
which come into play. With the attached patch I don't think we
can have any such problems with a *larger* BLCKSZ, so the only
point of the warning would be for a BLCKSZ of 4KB or less. Is it
worth carrying the warning code (with an appropriate adjustment
to the thresholds) or should we drop it?Is this still an open item?
Yes, although I'm not clear on whether people feel it is one which
needs to be fixed for 9.1 or left for 9.2.
On a build with a BLCKSZ less than 8KB we would not get a warning
before problems occurred, and we would have more serious problem
involving potentially incorrect behavior. Tom questioned whether
anyone actually builds with BLCKSZ less than 8KB, and I'm not
altogether sure that SLRUs dealing with transaction IDs would behave
correctly either.
On block sizes larger than 8KB it will the warning if you burn
through one billion transactions while holding one serializable read
write transaction open, even though there won't be a problem. With
the larger BLCKSZ values it may also generate log level messages
about SLRU wraparound when that's not really a problem.
The patch posted with the quoted message will prevent the
misbehavior on smaller block sizes and the bogus log messages on
larger block sizes. We'd need to change a couple more lines to get
the warning to trigger at the appropriate time for different block
sizes -- or we could just rip out the warning code. (I didn't post
a patch for that because there wasn't a clear consensus about
whether to fix it, rip it out, or leave it alone for 9.1.)
-Kevin
On Tue, Jul 5, 2011 at 10:51 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
Is this still an open item?
Yes, although I'm not clear on whether people feel it is one which
needs to be fixed for 9.1 or left for 9.2.On a build with a BLCKSZ less than 8KB we would not get a warning
before problems occurred, and we would have more serious problem
involving potentially incorrect behavior. Tom questioned whether
anyone actually builds with BLCKSZ less than 8KB, and I'm not
altogether sure that SLRUs dealing with transaction IDs would behave
correctly either.On block sizes larger than 8KB it will the warning if you burn
through one billion transactions while holding one serializable read
write transaction open, even though there won't be a problem. With
the larger BLCKSZ values it may also generate log level messages
about SLRU wraparound when that's not really a problem.
Well, as long as we can verify that OLDSERXID_MAX_PAGE has the same
value for BLCKSZ=8K before and after this patch, I don't see any real
downside to applying it. If, hypothetically, it's buggy, it's only
going to break things for non-default block sizes which are, by your
description, not correct right now anyway.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote:
Well, as long as we can verify that OLDSERXID_MAX_PAGE has the
same value for BLCKSZ=8K before and after this patch, I don't see
any real downside to applying it. If, hypothetically, it's buggy,
it's only going to break things for non-default block sizes which
are, by your description, not correct right now anyway.
Outside of a code comment, the entire patch consists of replacing
the definition of the OLDSERXID_MAX_PAGE macro. The old definition
is:
(SLRU_PAGES_PER_SEGMENT * 0x10000 - 1)
SLRU_PAGES_PER_SEGMENT is define to be 32. So this is:
(32 * 0x10000) - 1 = 2097151
The new definition is the min of the old one and a value based on
BLCKSZ:
(MaxTransactionId + 1) / OLDSERXID_ENTRIESPERPAGE - 1)
Where "entries per page" is BLCKSZ / sizeof(uint64).
For an 8K BLCKSZ this is:
((0xffffffff + 1) / 1024) - 1 = 4194303
So the macro is guaranteed to have the same value as it currently
does for BLCKSZ of 16KB or lower.
-Kevin
On Tue, Jul 5, 2011 at 12:03 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
Robert Haas <robertmhaas@gmail.com> wrote:
Well, as long as we can verify that OLDSERXID_MAX_PAGE has the
same value for BLCKSZ=8K before and after this patch, I don't see
any real downside to applying it. If, hypothetically, it's buggy,
it's only going to break things for non-default block sizes which
are, by your description, not correct right now anyway.Outside of a code comment, the entire patch consists of replacing
the definition of the OLDSERXID_MAX_PAGE macro. The old definition
is:(SLRU_PAGES_PER_SEGMENT * 0x10000 - 1)
SLRU_PAGES_PER_SEGMENT is define to be 32. So this is:
(32 * 0x10000) - 1 = 2097151
The new definition is the min of the old one and a value based on
BLCKSZ:(MaxTransactionId + 1) / OLDSERXID_ENTRIESPERPAGE - 1)
Where "entries per page" is BLCKSZ / sizeof(uint64).
For an 8K BLCKSZ this is:
((0xffffffff + 1) / 1024) - 1 = 4194303
So the macro is guaranteed to have the same value as it currently
does for BLCKSZ of 16KB or lower.
I went ahead and committed this.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company