Re: [BUGS] BUG #5915: OldSerXidAdd inflates pg_serial too much
Heikki Linnakangas wrote: Here's what I had in mind. Can you review The additions and modifications to the comments all look good to me. I can see why you renamed one field and eliminated another; no problem there. I'm really surprised, when you ignore those changes, how little was needed to move this to checkpoint time. I hadn't looked into that, and expected it to be much harder to do, even though I should know better by now. :-) I looked it over and pondered a bit, and have three concerns. (1) Why is it safe to assume that checkpoint will occur before this wraps around? Is it just that it takes a billion transactions, and one can reasonably expect a checkpoint before reaching that point, or is there some other safety net? (2) What if there are only occassional clusters of serializable transactions? If the one SLRU page is held so that it isn't repeatedly truncated and re-zeroed, does it pose a risk if transaction IDs wrap around while that page is held? (I don't think this is a new problem with the proposed patch, it just made it more obvious.) Should we be using RecentGlobalXmin or something in that checkpoint function to truncate that last page when it is safe to do so? (3) That unnecessary SLRU flush should probably be conditioned on a #ifdef or some not-very-visible GUC or something. With the problems which some people have with writes glutting the I/O during checkpoint I'd hate to add to the writes at checkpoint time just for debugging info -- at least without a specific request for that behavior. Other than that, it seems like a very good idea. do you have something to test this with? Well, I tested it with the src/test/isolation/ tests with and without TEST_OLDSERXID defined, so it passes that much[1]; but if Dan can grab another day on the MIT benchmarking server for a new round of DBT-2 tests (at least some of which should be run with TEST_OLDSERXID defined), it would help shake out any problems. I have to say, though, that Takashi-san seems to be the master of chasing down corner condition problems with this patch. Constructing test scripts for these corner cases which can be run under the normal PostgreSQL regression testing framework is hard, but I intend to continue to try to develop some before release. But as Dan pointed out recently, some of these issues are really hard to hit without pounding on the software for hours with high concurrency on a machine with a lot of CPUs. -Kevin [1] Actually, the last of the tests has been showing failure when running with TEST_OLDSERXID defined, because it gives up on the transaction set and forces a transaction to roll back a little earlier due to the summarization of the conflict data. I'm attaching another file we may want to add to src/test/isolation/expected to avoid confusion when people test a build with that defined. multiple-row-versions_1.out Description: Binary data -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #5915: OldSerXidAdd inflates pg_serial too much
On 04.03.2011 23:28, Kevin Grittner wrote:
I wrote:
I think what we're protecting against is disk I/O at COMMIT time,
not transaction startup.
One more thought on this -- on a properly configured server, this
code should rarely be exercised unless there is a long-running READ
WRITE transaction. The delay, if any, would be on the connection
which is committing that long running transaction.
What worries me most is that the cleanup happens while
SerializableXactHashLock is held. It's probably not a big deal in
practice, but it seems safer and probably more readable too to do the
cleanup at checkpoint.
Here's what I had in mind. Can you review, and do you have something to
test this with?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b4eb4ac..760c0ad 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -48,6 +48,7 @@
#include storage/ipc.h
#include storage/latch.h
#include storage/pmsignal.h
+#include storage/predicate.h
#include storage/procarray.h
#include storage/reinit.h
#include storage/smgr.h
@@ -7813,6 +7814,7 @@ CheckPointGuts(XLogRecPtr checkPointRedo, int flags)
CheckPointCLOG();
CheckPointSUBTRANS();
CheckPointMultiXact();
+ CheckPointPredicate();
CheckPointRelationMap();
CheckPointBuffers(flags); /* performs all required fsyncs */
/* We deliberately delay 2PC checkpointing as long as possible */
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 700c0db..9c40d4b 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -323,10 +323,9 @@ static SlruCtlData OldSerXidSlruCtlData;
typedef struct OldSerXidControlData
{
- int headPage;
- int tailSegment;
- TransactionId headXid;
- TransactionId tailXid;
+ int headPage; /* newest initialized page */
+ TransactionId headXid; /* newest valid Xid in the SLRU */
+ TransactionId tailXid; /* oldest xmin we might be interested in */
bool warningIssued;
} OldSerXidControlData;
@@ -711,7 +710,6 @@ OldSerXidInit(void)
* Set control information to reflect empty SLRU.
*/
oldSerXidControl-headPage = -1;
- oldSerXidControl-tailSegment = -1;
oldSerXidControl-headXid = InvalidTransactionId;
oldSerXidControl-tailXid = InvalidTransactionId;
oldSerXidControl-warningIssued = false;
@@ -722,10 +720,6 @@ OldSerXidInit(void)
* Record a committed read write serializable xid and the minimum
* commitSeqNo of any transactions to which this xid had a rw-conflict out.
* A zero seqNo means that there were no conflicts out from xid.
- *
- * The return value is normally false -- true means that we're about to
- * wrap around our space for tracking these xids, so the caller might want
- * to take action to prevent that.
*/
static void
OldSerXidAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo)
@@ -733,7 +727,7 @@ OldSerXidAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo)
TransactionId tailXid;
int targetPage;
int slotno;
- int page;
+ int firstZeroPage;
int xidSpread;
bool isNewPage;
@@ -745,30 +739,34 @@ OldSerXidAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo)
/*
* If no serializable transactions are active, there shouldn't be anything
- * to push out to this SLRU. Hitting this assert would mean there's
+ * to push out to the SLRU. Hitting this assert would mean there's
* something wrong with the earlier cleanup logic.
*/
tailXid = oldSerXidControl-tailXid;
Assert(TransactionIdIsValid(tailXid));
+ /*
+ * If the SLRU is currently unused, zero out the whole active region
+ * from tailXid to headXid before taking it into use. Otherwise zero
+ * out only any new pages that enter the tailXid-headXid range as we
+ * advance headXid.
+ */
if (oldSerXidControl-headPage 0)
{
- page = OldSerXidPage(tailXid);
- oldSerXidControl-tailSegment = OldSerXidSegment(page);
- page = oldSerXidControl-tailSegment * OLDSERXID_ENTRIESPERPAGE;
+ firstZeroPage = OldSerXidPage(tailXid);
isNewPage = true;
}
else
{
- page = OldSerXidNextPage(oldSerXidControl-headPage);
- isNewPage = OldSerXidPagePrecedesLogically(oldSerXidControl-headPage, targetPage);
+ firstZeroPage = OldSerXidNextPage(oldSerXidControl-headPage);
+ isNewPage = OldSerXidPagePrecedesLogically(oldSerXidControl-headPage,
+ targetPage);
}
if (!TransactionIdIsValid(oldSerXidControl-headXid)
|| TransactionIdFollows(xid, oldSerXidControl-headXid))
oldSerXidControl-headXid = xid;
- if (oldSerXidControl-headPage 0
- || OldSerXidPagePrecedesLogically(oldSerXidControl-headPage, targetPage))
+ if (isNewPage)
oldSerXidControl-headPage = targetPage;
xidSpread = (((uint32) xid) - ((uint32) tailXid));
@@ -788,10 +786,10 @@ OldSerXidAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo)
if (isNewPage)
[BUGS] BUG #5915: OldSerXidAdd inflates pg_serial too much
The following bug has been logged online:
Bug reference: 5915
Logged by: YAMAMOTO Takashi
Email address: y...@mwd.biglobe.ne.jp
PostgreSQL version: 9.1devel
Operating system: NetBSD
Description:OldSerXidAdd inflates pg_serial too much
Details:
a seemingly wrong math in OldSerXidAdd makes it busy writing zeros
to pg_serial.
diff --git a/src/backend/storage/lmgr/predicate.c
b/src/backend/storage/lmgr/predicate.c
index aa657fa..297508b 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -755,7 +755,7 @@ OldSerXidAdd(TransactionId xid, SerCommitSeqNo
minConflictCommitSeqNo)
{
page = OldSerXidPage(tailXid);
oldSerXidControl-tailSegment = OldSerXidSegment(page);
- page = oldSerXidControl-tailSegment * OLDSERXID_ENTRIESPERPAGE;
+ page = oldSerXidControl-tailSegment * SLRU_PAGES_PER_SEGMENT;
isNewPage = true;
}
else
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #5915: OldSerXidAdd inflates pg_serial too much
YAMAMOTO Takashi y...@mwd.biglobe.ne.jp wrote: a seemingly wrong math in OldSerXidAdd makes it busy writing zeros to pg_serial. [patch] Your fix looks correct to me -- we want to get from a SLRU segment number to the first page of that segment, so SLRU_PAGES_PER_SEGMENT is the right multiplier. Thanks! While I was looking at it, I noticed some obsolete comment lines which should be removed. Trivial patch attached. -Kevin *** a/src/backend/storage/lmgr/predicate.c --- b/src/backend/storage/lmgr/predicate.c *** *** 722,731 OldSerXidInit(void) * Record a committed read write serializable xid and the minimum * commitSeqNo of any transactions to which this xid had a rw-conflict out. * A zero seqNo means that there were no conflicts out from xid. - * - * The return value is normally false -- true means that we're about to - * wrap around our space for tracking these xids, so the caller might want - * to take action to prevent that. */ static void OldSerXidAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo) --- 722,727 -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #5915: OldSerXidAdd inflates pg_serial too much
On 04.03.2011 21:26, Kevin Grittner wrote: YAMAMOTO Takashiy...@mwd.biglobe.ne.jp wrote: a seemingly wrong math in OldSerXidAdd makes it busy writing zeros to pg_serial. [patch] Your fix looks correct to me -- we want to get from a SLRU segment number to the first page of that segment, so SLRU_PAGES_PER_SEGMENT is the right multiplier. Thanks! While I was looking at it, I noticed some obsolete comment lines which should be removed. Trivial patch attached. Hmm, if I'm reading that function correctly, it makes sure that when headPage 0 (which implies that the SLRU has not been used since startup, right? ), it zeroes out the whole SLRU file, not only the currently active region. At least pg_subtrans doesn't seem to bother with that, StartupSubTrans only zeroes the active region. I wonder if we should move the responsibility of truncating the SLRU to checkpoint. At the moment, it's done in OldSerXidSetActiveSerXmin(), while the callers are holding SerializableXactHashLock in exclusive mode. While it'll probably go quickly in practice, it still seems like there's a risk of stalling all new transactions for a few seconds while that happens. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #5915: OldSerXidAdd inflates pg_serial too much
Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Hmm, if I'm reading that function correctly, it makes sure that when headPage 0 (which implies that the SLRU has not been used since startup, right? ) No, look at the bottom of OldSerXidSetActiveSerXmin() -- cleanup of segments is done incrementally, but when it finds it has cleaned up *everything* it sets headPage = -1. I believe that should only happen when the xmin has moved past the end of the segment. it zeroes out the whole SLRU file, not only the currently active region. That's not the intent. If it's doing that, it's accidental. It is trying to zero from the start of a segment, if a new one is needed. I'll look at this some more to see if I can spot something I'm missing, but if you see something to indicate it's not working as I describe above, please point me in the right direction. -Kevin -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #5915: OldSerXidAdd inflates pg_serial too much
On 04.03.2011 22:33, Kevin Grittner wrote: Heikki Linnakangasheikki.linnakan...@enterprisedb.com wrote: Hmm, if I'm reading that function correctly, it makes sure that when headPage 0 (which implies that the SLRU has not been used since startup, right? ) No, look at the bottom of OldSerXidSetActiveSerXmin() -- cleanup of segments is done incrementally, but when it finds it has cleaned up *everything* it sets headPage = -1. I believe that should only happen when the xmin has moved past the end of the segment. it zeroes out the whole SLRU file, not only the currently active region. That's not the intent. If it's doing that, it's accidental. It is trying to zero from the start of a segment, if a new one is needed. Sorry, I was not entirely clear. It clears all pages from the start of the segment, up to the last currently active page, even if the active region from tailXid to headXid only spans a couple of pages somewhere in the middle of the segment. It seems pointless to clear the pages in the beginning of the segment in that case. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #5915: OldSerXidAdd inflates pg_serial too much
Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Sorry, I was not entirely clear. It clears all pages from the start of the segment, up to the last currently active page, even if the active region from tailXid to headXid only spans a couple of pages somewhere in the middle of the segment. It seems pointless to clear the pages in the beginning of the segment in that case. Oh, I see now. Somehow I thought I needed to clear pages from the start of the segment to the active portion. If that's not needed, it's easy enough to adjust the calculations there to start with the first active page. Do you want me to do that? -Kevin -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #5915: OldSerXidAdd inflates pg_serial too much
Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: I wonder if we should move the responsibility of truncating the SLRU to checkpoint. At the moment, it's done in OldSerXidSetActiveSerXmin(), while the callers are holding SerializableXactHashLock in exclusive mode. While it'll probably go quickly in practice, it still seems like there's a risk of stalling all new transactions for a few seconds while that happens. I don't think it can stall new transactions unless they are DEFERRABLE, but it might stall the COMMIT of the oldest serializable transaction -- if I'm thinking it through correctly. (If not, I have another energy drink I can crack open.) Here's my thinking: The call in RegisterSerializableTransactionInt() to OldSerXidSetActiveSerXmin() only happens if there are no active serializable transactions, so it can't fall into the SLRU truncation code (I think). I'm not worried about predicatelock_twophase_recover() because it will only call the function during startup with no transactions active, so it can't hit the cleanup code. That leaves SetNewSxactGlobalXmin(), which is only called from ReleasePredicateLocks(), and only when the last transaction with the low xmin is being cleaned up. Now, that's only called around completion of a transaction except when starting a SERIALIZABLE READ ONLY DEFERRABLE transaction; and if you have explicitly requested a DEFERRABLE transaction you presumably won't be astonished by a delay in its startup. So, I'm not arguing that we shouldn't move the truncation to checkpoint time, but I think what we're protecting against is disk I/O at COMMIT time, not transaction startup. Presumably disk I/O at that point would be less surprising, although perhaps the fact that it can happen on a read only transaction might surprise someone. Of course, moving any possible delay from this to a background process seems like a good thing in general; I just don't know whether it's worth doing right now, versus adding to a list of possible enhancements. If you're confident it's safe enough, I'm game. -Kevin -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #5915: OldSerXidAdd inflates pg_serial too much
I wrote: I think what we're protecting against is disk I/O at COMMIT time, not transaction startup. One more thought on this -- on a properly configured server, this code should rarely be exercised unless there is a long-running READ WRITE transaction. The delay, if any, would be on the connection which is committing that long running transaction. -Kevin -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
