pg_serial early wraparound
Hi hackers,
The SLRU managed by predicate.c can wrap around and overwrite data if
you have more than 1 billion active XIDs. That's because when SSI was
implemented, slru.c was limited to four digit segment names, which
implied a page limit that wasn't enough for pg_serial to have space
for every possible XID. We should probably rip that code out, because
SLRUs now support five digit segment names. Something like the
attached. I'll post a test script to demonstrate correct wraparound
behaviour around in time for one of the later CFs.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
ssi-slru-wraparound-v1.patchapplication/octet-stream; name=ssi-slru-wraparound-v1.patchDownload
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 24ed21b..6c3df2d 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -316,11 +316,9 @@ static SlruCtlData OldSerXidSlruCtlData;
#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.
+ * Set maximum pages based on the number needed to track all transactions.
*/
-#define OLDSERXID_MAX_PAGE Min(SLRU_PAGES_PER_SEGMENT * 0x10000 - 1, \
- (MaxTransactionId) / OLDSERXID_ENTRIESPERPAGE)
+#define OLDSERXID_MAX_PAGE ((MaxTransactionId) / OLDSERXID_ENTRIESPERPAGE)
#define OldSerXidNextPage(page) (((page) >= OLDSERXID_MAX_PAGE) ? 0 : (page) + 1)
@@ -328,7 +326,7 @@ static SlruCtlData OldSerXidSlruCtlData;
(OldSerXidSlruCtl->shared->page_buffer[slotno] + \
((((uint32) (xid)) % OLDSERXID_ENTRIESPERPAGE) * OLDSERXID_ENTRYSIZE))))
-#define OldSerXidPage(xid) ((((uint32) (xid)) / OLDSERXID_ENTRIESPERPAGE) % (OLDSERXID_MAX_PAGE + 1))
+#define OldSerXidPage(xid) (((uint32) (xid)) / OLDSERXID_ENTRIESPERPAGE)
#define OldSerXidSegment(page) ((page) / SLRU_PAGES_PER_SEGMENT)
typedef struct OldSerXidControlData
@@ -336,7 +334,6 @@ typedef struct OldSerXidControlData
int headPage; /* newest initialized page */
TransactionId headXid; /* newest valid Xid in the SLRU */
TransactionId tailXid; /* oldest xmin we might be interested in */
- bool warningIssued; /* have we issued SLRU wrap-around warning? */
} OldSerXidControlData;
typedef struct OldSerXidControlData *OldSerXidControl;
@@ -815,7 +812,6 @@ OldSerXidInit(void)
oldSerXidControl->headPage = -1;
oldSerXidControl->headXid = InvalidTransactionId;
oldSerXidControl->tailXid = InvalidTransactionId;
- oldSerXidControl->warningIssued = false;
}
}
@@ -871,47 +867,6 @@ OldSerXidAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo)
if (isNewPage)
oldSerXidControl->headPage = targetPage;
- /*
- * Give a warning if we're about to run out of SLRU pages.
- *
- * slru.c has a maximum of 64k segments, with 32 (SLRU_PAGES_PER_SEGMENT)
- * pages each. We need to store a 64-bit integer for each Xid, and with
- * default 8k block size, 65536*32 pages is only enough to cover 2^30
- * XIDs. If we're about to hit that limit and wrap around, warn the user.
- *
- * To avoid spamming the user, we only give one warning when we've used 1
- * billion XIDs, and stay silent until the situation is fixed and the
- * number of XIDs used falls below 800 million again.
- *
- * XXX: We have no safeguard to actually *prevent* the wrap-around,
- * though. All you get is a warning.
- */
- if (oldSerXidControl->warningIssued)
- {
- TransactionId lowWatermark;
-
- lowWatermark = tailXid + 800000000;
- if (lowWatermark < FirstNormalTransactionId)
- lowWatermark = FirstNormalTransactionId;
- if (TransactionIdPrecedes(xid, lowWatermark))
- oldSerXidControl->warningIssued = false;
- }
- else
- {
- TransactionId highWatermark;
-
- highWatermark = tailXid + 1000000000;
- if (highWatermark < FirstNormalTransactionId)
- highWatermark = FirstNormalTransactionId;
- if (TransactionIdFollows(xid, highWatermark))
- {
- oldSerXidControl->warningIssued = true;
- ereport(WARNING,
- (errmsg("memory for serializable conflict tracking is nearly exhausted"),
- errhint("There might be an idle transaction or a forgotten prepared transaction causing this.")));
- }
- }
-
if (isNewPage)
{
/* Initialize intervening pages. */
On Wed, Nov 9, 2016 at 11:07 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
The SLRU managed by predicate.c can wrap around and overwrite data if
you have more than 1 billion active XIDs. That's because when SSI was
implemented, slru.c was limited to four digit segment names, which
implied a page limit that wasn't enough for pg_serial to have space
for every possible XID. We should probably rip that code out, because
SLRUs now support five digit segment names. Something like the
attached. I'll post a test script to demonstrate correct wraparound
behaviour around in time for one of the later CFs.
Here is a shell script that shows a full rotation through xid space if
you build PostgreSQL with TEST_OLDSERXID, which you can do by
uncommenting a line in predicate.c. On master we see the SLRU
segments go around the clock twice for each time xid goes around.
With the patch it goes around just once, adding an extra character to
the segment name to double the space.
By the way, I think the real number of xids it can hold today is
(65536 * 32 * 8192) / sizeof(uint64) = 2^31 xids, not 2^30 as
indicated by an existing comment. So I think there is actually enough
space and wrapping is probably harmess, but it seems cleaner and
simpler not to do that and to rip out the scary warning code, so I'll
add this to the CF.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
On Mon, Feb 27, 2017 at 7:28 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
On Wed, Nov 9, 2016 at 11:07 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:The SLRU managed by predicate.c can wrap around and overwrite data if
you have more than 1 billion active XIDs. That's because when SSI was
implemented, slru.c was limited to four digit segment names, which
implied a page limit that wasn't enough for pg_serial to have space
for every possible XID. We should probably rip that code out, because
SLRUs now support five digit segment names. Something like the
attached. I'll post a test script to demonstrate correct wraparound
behaviour around in time for one of the later CFs.Here is a shell script that shows a full rotation through xid space if
you build PostgreSQL with TEST_OLDSERXID, which you can do by
uncommenting a line in predicate.c. On master we see the SLRU
segments go around the clock twice for each time xid goes around.
With the patch it goes around just once, adding an extra character to
the segment name to double the space.
I attached the wrong version. Here is the right one.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Hi, I've tried to review this patch, but it seems that I miss something essential.
You claim that SLRUs now support five digit segment name, while in slru.h
at current master I see the following:
* Note: slru.c currently assumes that segment file names will be four hex
* digits. This sets a lower bound on the segment size (64K transactions
* for 32-bit TransactionIds).
*/
#define SLRU_PAGES_PER_SEGMENT 32
/* Maximum length of an SLRU name */
#define SLRU_MAX_NAME_LENGTH 32
Could you please clarify the idea of the patch? Is it still relevant?
I've also run your test script.
pg_clog was renamed to pg_xact, so it need to be changed accordingly
echo "Contents of pg_clog:"
ls $PGDATA/pg_xact/
The test shows failed assertion:
========== setting next xid to 1073741824 =========
Transaction log reset
waiting for server to start....2017-03-24 17:05:19.897 MSK [1181] LOG: listening on IPv4 address "127.0.0.1", port 5432
2017-03-24 17:05:19.981 MSK [1181] LOG: listening on Unix socket "/tmp/.s.PGSQL.5432"
2017-03-24 17:05:20.081 MSK [1183] LOG: database system was shut down at 2017-03-24 17:05:19 MSK
2017-03-24 17:05:20.221 MSK [1181] LOG: database system is ready to accept connections
done
server started
vacuumdb: vacuuming database "postgres"
vacuumdb: vacuuming database "template0"
vacuumdb: vacuuming database "template1"
TRAP: FailedAssertion("!(TransactionIdPrecedesOrEquals(oldestXact, ShmemVariableCache->oldestXid))", File: "clog.c", Line: 669)
vacuumdb: vacuuming of database "template1" failed: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
2017-03-24 17:05:21.541 MSK [1181] LOG: server process (PID 1202) was terminated by signal 6: Aborted
2017-03-24 17:05:21.541 MSK [1181] DETAIL: Failed process was running: VACUUM (FREEZE);
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Mar 25, 2017 at 3:11 AM, Anastasia Lubennikova
<lubennikovaav@gmail.com> wrote:
Hi, I've tried to review this patch, but it seems that I miss something essential.
Hi Anastasia,
Thanks for looking at this.
You claim that SLRUs now support five digit segment name, while in slru.h
at current master I see the following:* Note: slru.c currently assumes that segment file names will be four hex
* digits. This sets a lower bound on the segment size (64K transactions
* for 32-bit TransactionIds).
*/
#define SLRU_PAGES_PER_SEGMENT 32/* Maximum length of an SLRU name */
#define SLRU_MAX_NAME_LENGTH 32
That comment is out of date. Commit 638cf09e extended SLRUs to
support 5 digit names, to support pg_multixact. And I see now that
commit 73c986ad more recently created the possibility of 6 chacater
SLRU file names for pg_commit_ts.
Could you please clarify the idea of the patch? Is it still relevant?
The idea is simply to remove some strange old code including scary
error messages that is no longer needed. In my study of predicate.c
for other reasons, I noticed this in passing and thought I'd tidy it
up. Because I have tangled with pg_multixact and seen 5-character
SLRU files with my own eyes, I knew that the restriction that
motivated this code was no longer valid.
I've also run your test script.
pg_clog was renamed to pg_xact, so it need to be changed accordingly
echo "Contents of pg_clog:"
ls $PGDATA/pg_xact/
Right.
The test shows failed assertion:
========== setting next xid to 1073741824 =========
Transaction log reset
waiting for server to start....2017-03-24 17:05:19.897 MSK [1181] LOG: listening on IPv4 address "127.0.0.1", port 5432
2017-03-24 17:05:19.981 MSK [1181] LOG: listening on Unix socket "/tmp/.s.PGSQL.5432"
2017-03-24 17:05:20.081 MSK [1183] LOG: database system was shut down at 2017-03-24 17:05:19 MSK
2017-03-24 17:05:20.221 MSK [1181] LOG: database system is ready to accept connections
done
server started
vacuumdb: vacuuming database "postgres"
vacuumdb: vacuuming database "template0"
vacuumdb: vacuuming database "template1"
TRAP: FailedAssertion("!(TransactionIdPrecedesOrEquals(oldestXact, ShmemVariableCache->oldestXid))", File: "clog.c", Line: 669)
vacuumdb: vacuuming of database "template1" failed: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
2017-03-24 17:05:21.541 MSK [1181] LOG: server process (PID 1202) was terminated by signal 6: Aborted
2017-03-24 17:05:21.541 MSK [1181] DETAIL: Failed process was running: VACUUM (FREEZE);
My cheap trick for moving the xid around the clock quickly to test
wraparound scenarios no longer works, since this new assertion was
added in ea42cc18. That was committed just a few hours before you
tested this. Bad luck for me!
The new status of this patch is: Waiting on Author
It's not urgent, it's just cleanup work, so I've now moved it to the
next commitfest. I will try to figure out a new way to demonstrate
that it works correctly without having to ask a reviewing to disable
any assertions. Thanks again.
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Mar 25, 2017 at 7:27 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
On Sat, Mar 25, 2017 at 3:11 AM, Anastasia Lubennikova
<lubennikovaav@gmail.com> wrote:You claim that SLRUs now support five digit segment name, while in slru.h
at current master I see the following:* Note: slru.c currently assumes that segment file names will be four hex
* digits. This sets a lower bound on the segment size (64K transactions
* for 32-bit TransactionIds).
*/
I've now complained about that comment in a separate thread.
It's not urgent, it's just cleanup work, so I've now moved it to the
next commitfest. I will try to figure out a new way to demonstrate
that it works correctly without having to ask a review[er] to disable
any assertions. Thanks again.
Here's a rebased batch.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
ssi-slru-wraparound-v2.patchapplication/octet-stream; name=ssi-slru-wraparound-v2.patchDownload
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index a4cb4d33add..bbc4b54f684 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -316,11 +316,9 @@ static SlruCtlData OldSerXidSlruCtlData;
#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.
+ * Set maximum pages based on the number needed to track all transactions.
*/
-#define OLDSERXID_MAX_PAGE Min(SLRU_PAGES_PER_SEGMENT * 0x10000 - 1, \
- (MaxTransactionId) / OLDSERXID_ENTRIESPERPAGE)
+#define OLDSERXID_MAX_PAGE ((MaxTransactionId) / OLDSERXID_ENTRIESPERPAGE)
#define OldSerXidNextPage(page) (((page) >= OLDSERXID_MAX_PAGE) ? 0 : (page) + 1)
@@ -328,7 +326,7 @@ static SlruCtlData OldSerXidSlruCtlData;
(OldSerXidSlruCtl->shared->page_buffer[slotno] + \
((((uint32) (xid)) % OLDSERXID_ENTRIESPERPAGE) * OLDSERXID_ENTRYSIZE))))
-#define OldSerXidPage(xid) ((((uint32) (xid)) / OLDSERXID_ENTRIESPERPAGE) % (OLDSERXID_MAX_PAGE + 1))
+#define OldSerXidPage(xid) (((uint32) (xid)) / OLDSERXID_ENTRIESPERPAGE)
#define OldSerXidSegment(page) ((page) / SLRU_PAGES_PER_SEGMENT)
typedef struct OldSerXidControlData
@@ -336,7 +334,6 @@ typedef struct OldSerXidControlData
int headPage; /* newest initialized page */
TransactionId headXid; /* newest valid Xid in the SLRU */
TransactionId tailXid; /* oldest xmin we might be interested in */
- bool warningIssued; /* have we issued SLRU wrap-around warning? */
} OldSerXidControlData;
typedef struct OldSerXidControlData *OldSerXidControl;
@@ -823,7 +820,6 @@ OldSerXidInit(void)
oldSerXidControl->headPage = -1;
oldSerXidControl->headXid = InvalidTransactionId;
oldSerXidControl->tailXid = InvalidTransactionId;
- oldSerXidControl->warningIssued = false;
}
}
@@ -879,47 +875,6 @@ OldSerXidAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo)
if (isNewPage)
oldSerXidControl->headPage = targetPage;
- /*
- * Give a warning if we're about to run out of SLRU pages.
- *
- * slru.c has a maximum of 64k segments, with 32 (SLRU_PAGES_PER_SEGMENT)
- * pages each. We need to store a 64-bit integer for each Xid, and with
- * default 8k block size, 65536*32 pages is only enough to cover 2^30
- * XIDs. If we're about to hit that limit and wrap around, warn the user.
- *
- * To avoid spamming the user, we only give one warning when we've used 1
- * billion XIDs, and stay silent until the situation is fixed and the
- * number of XIDs used falls below 800 million again.
- *
- * XXX: We have no safeguard to actually *prevent* the wrap-around,
- * though. All you get is a warning.
- */
- if (oldSerXidControl->warningIssued)
- {
- TransactionId lowWatermark;
-
- lowWatermark = tailXid + 800000000;
- if (lowWatermark < FirstNormalTransactionId)
- lowWatermark = FirstNormalTransactionId;
- if (TransactionIdPrecedes(xid, lowWatermark))
- oldSerXidControl->warningIssued = false;
- }
- else
- {
- TransactionId highWatermark;
-
- highWatermark = tailXid + 1000000000;
- if (highWatermark < FirstNormalTransactionId)
- highWatermark = FirstNormalTransactionId;
- if (TransactionIdFollows(xid, highWatermark))
- {
- oldSerXidControl->warningIssued = true;
- ereport(WARNING,
- (errmsg("memory for serializable conflict tracking is nearly exhausted"),
- errhint("There might be an idle transaction or a forgotten prepared transaction causing this.")));
- }
- }
-
if (isNewPage)
{
/* Initialize intervening pages. */
On Wed, Jun 28, 2017 at 1:11 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
On Sat, Mar 25, 2017 at 7:27 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:On Sat, Mar 25, 2017 at 3:11 AM, Anastasia Lubennikova
<lubennikovaav@gmail.com> wrote:You claim that SLRUs now support five digit segment name, while in slru.h
at current master I see the following:* Note: slru.c currently assumes that segment file names will be four hex
* digits. This sets a lower bound on the segment size (64K transactions
* for 32-bit TransactionIds).
*/I've now complained about that comment in a separate thread.
It's not urgent, it's just cleanup work, so I've now moved it to the
next commitfest. I will try to figure out a new way to demonstrate
that it works correctly without having to ask a review[er] to disable
any assertions. Thanks again.
Rebased again, now with a commit message. That assertion has since
been removed (commit ec99dd5a) so the attached test script can once
again be used to see the contents of pg_serial as the xid goes all the
way around, if you build with TEST_OLDSERXID defined so that
predicate.c forces information about xids out to pg_serial.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
ssi-slru-wraparound-v3.patchapplication/octet-stream; name=ssi-slru-wraparound-v3.patchDownload
From a098871d02b4883bfa531cb6e20ab4b66a6b80e0 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Fri, 1 Sep 2017 22:39:35 +1200
Subject: [PATCH] Remove obsolete SLRU wrapping and warnings from predicate.c.
When SSI was developed slru.c was limited to segments files with names in the
range 0000..FFFF. This didn't allow enough space for predicate.c to store
every possible xid when spilling old transactions to disk, so it would wrap
around sooner and print warnings. Since commits 638cf09e and 73c986ad
increased the number of segments files slru.c could manage, that behavior is
unnecessary. Therefore remove that code.
Author: Thomas Munro
Reviewed-By: Anastasia Lubennikova
Discussion: https://postgr.es/m/CAEepm=3XfsTSxgEbEOmxu0QDiXy0o18NUg2nC89JZcCGE+XFPA@mail.gmail.com
---
src/backend/storage/lmgr/predicate.c | 51 +++---------------------------------
1 file changed, 3 insertions(+), 48 deletions(-)
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 6a6d9d6d5cc..5d4374cb06f 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -318,11 +318,9 @@ static SlruCtlData OldSerXidSlruCtlData;
#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.
+ * Set maximum pages based on the number needed to track all transactions.
*/
-#define OLDSERXID_MAX_PAGE Min(SLRU_PAGES_PER_SEGMENT * 0x10000 - 1, \
- (MaxTransactionId) / OLDSERXID_ENTRIESPERPAGE)
+#define OLDSERXID_MAX_PAGE ((MaxTransactionId) / OLDSERXID_ENTRIESPERPAGE)
#define OldSerXidNextPage(page) (((page) >= OLDSERXID_MAX_PAGE) ? 0 : (page) + 1)
@@ -330,7 +328,7 @@ static SlruCtlData OldSerXidSlruCtlData;
(OldSerXidSlruCtl->shared->page_buffer[slotno] + \
((((uint32) (xid)) % OLDSERXID_ENTRIESPERPAGE) * OLDSERXID_ENTRYSIZE))))
-#define OldSerXidPage(xid) ((((uint32) (xid)) / OLDSERXID_ENTRIESPERPAGE) % (OLDSERXID_MAX_PAGE + 1))
+#define OldSerXidPage(xid) (((uint32) (xid)) / OLDSERXID_ENTRIESPERPAGE)
#define OldSerXidSegment(page) ((page) / SLRU_PAGES_PER_SEGMENT)
typedef struct OldSerXidControlData
@@ -338,7 +336,6 @@ typedef struct OldSerXidControlData
int headPage; /* newest initialized page */
TransactionId headXid; /* newest valid Xid in the SLRU */
TransactionId tailXid; /* oldest xmin we might be interested in */
- bool warningIssued; /* have we issued SLRU wrap-around warning? */
} OldSerXidControlData;
typedef struct OldSerXidControlData *OldSerXidControl;
@@ -826,7 +823,6 @@ OldSerXidInit(void)
oldSerXidControl->headPage = -1;
oldSerXidControl->headXid = InvalidTransactionId;
oldSerXidControl->tailXid = InvalidTransactionId;
- oldSerXidControl->warningIssued = false;
}
}
@@ -882,47 +878,6 @@ OldSerXidAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo)
if (isNewPage)
oldSerXidControl->headPage = targetPage;
- /*
- * Give a warning if we're about to run out of SLRU pages.
- *
- * slru.c has a maximum of 64k segments, with 32 (SLRU_PAGES_PER_SEGMENT)
- * pages each. We need to store a 64-bit integer for each Xid, and with
- * default 8k block size, 65536*32 pages is only enough to cover 2^30
- * XIDs. If we're about to hit that limit and wrap around, warn the user.
- *
- * To avoid spamming the user, we only give one warning when we've used 1
- * billion XIDs, and stay silent until the situation is fixed and the
- * number of XIDs used falls below 800 million again.
- *
- * XXX: We have no safeguard to actually *prevent* the wrap-around,
- * though. All you get is a warning.
- */
- if (oldSerXidControl->warningIssued)
- {
- TransactionId lowWatermark;
-
- lowWatermark = tailXid + 800000000;
- if (lowWatermark < FirstNormalTransactionId)
- lowWatermark = FirstNormalTransactionId;
- if (TransactionIdPrecedes(xid, lowWatermark))
- oldSerXidControl->warningIssued = false;
- }
- else
- {
- TransactionId highWatermark;
-
- highWatermark = tailXid + 1000000000;
- if (highWatermark < FirstNormalTransactionId)
- highWatermark = FirstNormalTransactionId;
- if (TransactionIdFollows(xid, highWatermark))
- {
- oldSerXidControl->warningIssued = true;
- ereport(WARNING,
- (errmsg("memory for serializable conflict tracking is nearly exhausted"),
- errhint("There might be an idle transaction or a forgotten prepared transaction causing this.")));
- }
- }
-
if (isNewPage)
{
/* Initialize intervening pages. */
--
2.13.2
On Fri, Sep 1, 2017 at 8:12 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
Rebased again, now with a commit message. That assertion has since
been removed (commit ec99dd5a) so the attached test script can once
again be used to see the contents of pg_serial as the xid goes all the
way around, if you build with TEST_OLDSERXID defined so that
predicate.c forces information about xids out to pg_serial.
Moved to next CF per lack of reviews.
--
Michael
Greetings,
* Thomas Munro (thomas.munro@enterprisedb.com) wrote:
On Wed, Jun 28, 2017 at 1:11 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:On Sat, Mar 25, 2017 at 7:27 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:On Sat, Mar 25, 2017 at 3:11 AM, Anastasia Lubennikova
<lubennikovaav@gmail.com> wrote:You claim that SLRUs now support five digit segment name, while in slru.h
at current master I see the following:* Note: slru.c currently assumes that segment file names will be four hex
* digits. This sets a lower bound on the segment size (64K transactions
* for 32-bit TransactionIds).
*/I've now complained about that comment in a separate thread.
It's not urgent, it's just cleanup work, so I've now moved it to the
next commitfest. I will try to figure out a new way to demonstrate
that it works correctly without having to ask a review[er] to disable
any assertions. Thanks again.Rebased again, now with a commit message. That assertion has since
been removed (commit ec99dd5a) so the attached test script can once
again be used to see the contents of pg_serial as the xid goes all the
way around, if you build with TEST_OLDSERXID defined so that
predicate.c forces information about xids out to pg_serial.
I've taken a look through this and it seems pretty reasonable. Would be
great to have someone actually try to duplicate the testing that Thomas
did (though I have little doubt that it works as described) and get it
to Ready-For-Committer state.
Anastasia, thanks for the previous review, any chance you could try
again with the latest patch (against the current state of git)?
Thanks!
Stephen
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
The patch doesn't break anything in regression tests and does the code cleanup.
As far as I understand, the removed code was dead, since SLRU size is large enough
and the wraparound, described in the message is impossible.
So I mark it as Ready For Committer.
I didn't manage to repeat the attached test, though.
Server doesn't start after xid reset. It throws an error:
server stopped
========== setting next xid to 1073741824 =========
Write-ahead log reset
waiting for server to start....2018-03-12 14:18:59.551 MSK [16126] LOG: listening on IPv4 address "127.0.0.1", port 5432
2018-03-12 14:18:59.625 MSK [16126] LOG: listening on Unix socket "/tmp/.s.PGSQL.5432"
2018-03-12 14:18:59.764 MSK [16127] LOG: database system was shut down at 2018-03-12 14:18:59 MSK
2018-03-12 14:18:59.802 MSK [16127] FATAL: could not access status of transaction 10737418
2018-03-12 14:18:59.802 MSK [16127] DETAIL: Could not open file "pg_xact/000A": Нет такого файла или каталога.
2018-03-12 14:18:59.803 MSK [16126] LOG: startup process (PID 16127) exited with exit code 1
2018-03-12 14:18:59.803 MSK [16126] LOG: aborting startup due to startup process failure
2018-03-12 14:18:59.804 MSK [16126] LOG: database system is shut down
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Rebased again, now with a commit message. That assertion has since
been removed (commit ec99dd5a) so the attached test script can once
again be used to see the contents of pg_serial as the xid goes all the
way around, if you build with TEST_OLDSERXID defined so that
predicate.c forces information about xids out to pg_serial.
Couple thoughts here ---
Seems like if the patch is correct as-is, then the OldSerXidPage
macro could be simplified, as the modulo no longer does anything.
Also, OldSerXidSegment doesn't seem to be used.
I'm a little worried because Anastasia couldn't repeat the test;
why is that?
regards, tom lane
On Tue, Mar 27, 2018 at 5:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Rebased again, now with a commit message. That assertion has since
been removed (commit ec99dd5a) so the attached test script can once
again be used to see the contents of pg_serial as the xid goes all the
way around, if you build with TEST_OLDSERXID defined so that
predicate.c forces information about xids out to pg_serial.Couple thoughts here ---
Thanks for looking at this!
Seems like if the patch is correct as-is, then the OldSerXidPage
macro could be simplified, as the modulo no longer does anything.
The patch already did that:
-#define OldSerXidPage(xid) ((((uint32) (xid)) /
OLDSERXID_ENTRIESPERPAGE) % (OLDSERXID_MAX_PAGE + 1))
+#define OldSerXidPage(xid) (((uint32) (xid)) / OLDSERXID_ENTRIESPERPAGE)Also, OldSerXidSegment doesn't seem to be used.
Right, thanks. Removed.
I'm a little worried because Anastasia couldn't repeat the test;
why is that?
Hmm. I'm not sure. It works for me on a couple of machines and what I see is:
========== setting next xid to 65536 =========
...
Contents of pg_serial:
0002
========== setting next xid to 1073741824 =========
...
Contents of pg_serial:
8000
========== setting next xid to 2147483648 =========
...
Contents of pg_serial:
10000
========== setting next xid to 3221225472 =========
...
Contents of pg_serial:
18000
========== setting next xid to 65536 =========
...
Contents of pg_serial:
0002
========== setting next xid to 1073741824 =========
...
Contents of pg_serial:
8000
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
0001-Remove-obsolete-SLRU-wrapping-and-warnings-from-p-v4.patchapplication/octet-stream; name=0001-Remove-obsolete-SLRU-wrapping-and-warnings-from-p-v4.patchDownload
From 0aa56351fd906bc0937140cbd7b828bbf858081f Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Fri, 1 Sep 2017 22:39:35 +1200
Subject: [PATCH] Remove obsolete SLRU wrapping and warnings from predicate.c.
When SSI was developed, slru.c was limited to segment files with names in the
range 0000-FFFF. This didn't allow enough space for predicate.c to store
every possible xid when spilling old transactions to disk, so it would wrap
around sooner and print warnings. Since commits 638cf09e and 73c986ad
increased the number of segment files slru.c could manage, that behavior is
unnecessary. Therefore remove that code.
Also remove the macro OldSerXidSegment, which has been unused since 4cd3fb6e.
Author: Thomas Munro
Reviewed-By: Anastasia Lubennikova, Tom Lane
Discussion: https://postgr.es/m/CAEepm=3XfsTSxgEbEOmxu0QDiXy0o18NUg2nC89JZcCGE+XFPA@mail.gmail.com
---
src/backend/storage/lmgr/predicate.c | 52 +++---------------------------------
1 file changed, 3 insertions(+), 49 deletions(-)
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 654eca4f3f..ecfa33e0a0 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -318,11 +318,9 @@ static SlruCtlData OldSerXidSlruCtlData;
#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.
+ * Set maximum pages based on the number needed to track all transactions.
*/
-#define OLDSERXID_MAX_PAGE Min(SLRU_PAGES_PER_SEGMENT * 0x10000 - 1, \
- (MaxTransactionId) / OLDSERXID_ENTRIESPERPAGE)
+#define OLDSERXID_MAX_PAGE ((MaxTransactionId) / OLDSERXID_ENTRIESPERPAGE)
#define OldSerXidNextPage(page) (((page) >= OLDSERXID_MAX_PAGE) ? 0 : (page) + 1)
@@ -330,15 +328,13 @@ static SlruCtlData OldSerXidSlruCtlData;
(OldSerXidSlruCtl->shared->page_buffer[slotno] + \
((((uint32) (xid)) % OLDSERXID_ENTRIESPERPAGE) * OLDSERXID_ENTRYSIZE))))
-#define OldSerXidPage(xid) ((((uint32) (xid)) / OLDSERXID_ENTRIESPERPAGE) % (OLDSERXID_MAX_PAGE + 1))
-#define OldSerXidSegment(page) ((page) / SLRU_PAGES_PER_SEGMENT)
+#define OldSerXidPage(xid) (((uint32) (xid)) / OLDSERXID_ENTRIESPERPAGE)
typedef struct OldSerXidControlData
{
int headPage; /* newest initialized page */
TransactionId headXid; /* newest valid Xid in the SLRU */
TransactionId tailXid; /* oldest xmin we might be interested in */
- bool warningIssued; /* have we issued SLRU wrap-around warning? */
} OldSerXidControlData;
typedef struct OldSerXidControlData *OldSerXidControl;
@@ -826,7 +822,6 @@ OldSerXidInit(void)
oldSerXidControl->headPage = -1;
oldSerXidControl->headXid = InvalidTransactionId;
oldSerXidControl->tailXid = InvalidTransactionId;
- oldSerXidControl->warningIssued = false;
}
}
@@ -882,47 +877,6 @@ OldSerXidAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo)
if (isNewPage)
oldSerXidControl->headPage = targetPage;
- /*
- * Give a warning if we're about to run out of SLRU pages.
- *
- * slru.c has a maximum of 64k segments, with 32 (SLRU_PAGES_PER_SEGMENT)
- * pages each. We need to store a 64-bit integer for each Xid, and with
- * default 8k block size, 65536*32 pages is only enough to cover 2^30
- * XIDs. If we're about to hit that limit and wrap around, warn the user.
- *
- * To avoid spamming the user, we only give one warning when we've used 1
- * billion XIDs, and stay silent until the situation is fixed and the
- * number of XIDs used falls below 800 million again.
- *
- * XXX: We have no safeguard to actually *prevent* the wrap-around,
- * though. All you get is a warning.
- */
- if (oldSerXidControl->warningIssued)
- {
- TransactionId lowWatermark;
-
- lowWatermark = tailXid + 800000000;
- if (lowWatermark < FirstNormalTransactionId)
- lowWatermark = FirstNormalTransactionId;
- if (TransactionIdPrecedes(xid, lowWatermark))
- oldSerXidControl->warningIssued = false;
- }
- else
- {
- TransactionId highWatermark;
-
- highWatermark = tailXid + 1000000000;
- if (highWatermark < FirstNormalTransactionId)
- highWatermark = FirstNormalTransactionId;
- if (TransactionIdFollows(xid, highWatermark))
- {
- oldSerXidControl->warningIssued = true;
- ereport(WARNING,
- (errmsg("memory for serializable conflict tracking is nearly exhausted"),
- errhint("There might be an idle transaction or a forgotten prepared transaction causing this.")));
- }
- }
-
if (isNewPage)
{
/* Initialize intervening pages. */
--
2.16.2
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Tue, Mar 27, 2018 at 5:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm a little worried because Anastasia couldn't repeat the test;
why is that?
Hmm. I'm not sure. It works for me on a couple of machines and what I see is:
Yeah, it works for me too, so not sure what the problem was.
The only other point I can think of is that by changing the storage
layout under pg_serial/, this'd create an issue for pg_upgrade, if
pg_upgrade tried to migrate this data. But it doesn't.
Hence, pushed.
regards, tom lane