relaxing sync commit if no WAL written (was Re: unlogged tables)
On Wed, Dec 15, 2010 at 2:20 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
Looks ok. I'd suggest rewording this comment though:
[ the comment in question ]
It's a bit hard to follow, as it first lists exceptions on when we must
flush XLOG immediately, and then lists conditions on when we can skip it.
See if the attached looks better to you. I mostly adopted your
proposal, with a bit of additional wordsmithing, and I also added a
parenthetical comment about why we don't skip writing the commit
record altogether in this case, since that's come up twice now.
I've removed the references to unlogged tables for now, as I'm
thinking it makes sense to commit this part first.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
relax-sync-commit-v2.patchapplication/octet-stream; name=relax-sync-commit-v2.patchDownload
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 79c9c0d..67f817c 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -907,6 +907,7 @@ RecordTransactionCommit(void)
int nmsgs = 0;
SharedInvalidationMessage *invalMessages = NULL;
bool RelcacheInitFileInval = false;
+ bool wrote_xlog;
/* Get data needed for commit record */
nrels = smgrGetPendingDeletes(true, &rels);
@@ -914,6 +915,7 @@ RecordTransactionCommit(void)
if (XLogStandbyInfoActive())
nmsgs = xactGetCommittedInvalidationMessages(&invalMessages,
&RelcacheInitFileInval);
+ wrote_xlog = (XactLastRecEnd.xrecoff != 0);
/*
* If we haven't been assigned an XID yet, we neither can, nor do we want
@@ -940,7 +942,7 @@ RecordTransactionCommit(void)
* assigned is a sequence advance record due to nextval() --- we want
* to flush that to disk before reporting commit.)
*/
- if (XactLastRecEnd.xrecoff == 0)
+ if (!wrote_xlog)
goto cleanup;
}
else
@@ -1028,16 +1030,27 @@ RecordTransactionCommit(void)
}
/*
- * Check if we want to commit asynchronously. If the user has set
- * synchronous_commit = off, and we're not doing cleanup of any non-temp
- * rels nor committing any command that wanted to force sync commit, then
- * we can defer flushing XLOG. (We must not allow asynchronous commit if
- * there are any non-temp tables to be deleted, because we might delete
- * the files before the COMMIT record is flushed to disk. We do allow
- * asynchronous commit if all to-be-deleted tables are temporary though,
- * since they are lost anyway if we crash.)
+ * Check if we want to commit asynchronously. We can allow the XLOG flush
+ * to happen asynchronously if synchronous_commit=off, or if the current
+ * transaction has not performed any WAL-logged operation. The latter case
+ * can arise if the current transaction wrote only to temporary tables.
+ * In case of a crash, the loss of such a transaction will be irrelevant
+ * since temp tables will be lost anyway. (Given the foregoing, you might
+ * think that it would be unnecessary to emit the XLOG record at all in
+ * this case, but we don't currently try to do that. It would certainly
+ * cause problems at least in Hot Standby mode, where the KnownAssignedXids
+ * machinery requires tracking every XID assignment. It might be OK to
+ * skip it only when wal_level < hot_standby, but for now we don't.)
+ *
+ * However, if we're doing cleanup of any non-temp rels or committing any
+ * command that wanted to force sync commit, then we must flush XLOG
+ * immediately. (We must not allow asynchronous commit if there are any
+ * non-temp tables to be deleted, because we might delete the files before
+ * the COMMIT record is flushed to disk. We do allow asynchronous commit
+ * if all to-be-deleted tables are temporary though, since they are lost
+ * anyway if we crash.)
*/
- if (XactSyncCommit || forceSyncCommit || nrels > 0)
+ if ((wrote_xlog && XactSyncCommit) || forceSyncCommit || nrels > 0)
{
/*
* Synchronous commit case:
On Fri, 2010-12-17 at 13:35 -0500, Robert Haas wrote:
I'm
thinking it makes sense to commit this part first.
This will break Hot Standby, as previously explained. Don't.
--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services
On Sun, Dec 19, 2010 at 7:01 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
On Fri, 2010-12-17 at 13:35 -0500, Robert Haas wrote:
I'm
thinking it makes sense to commit this part first.This will break Hot Standby, as previously explained. Don't.
Uh, why? Skipping the commit record altogether would do that, but
this patch doesn't do that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Sun, 2010-12-19 at 07:33 -0500, Robert Haas wrote:
On Sun, Dec 19, 2010 at 7:01 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
On Fri, 2010-12-17 at 13:35 -0500, Robert Haas wrote:
I'm
thinking it makes sense to commit this part first.This will break Hot Standby, as previously explained. Don't.
Uh, why? Skipping the commit record altogether would do that, but
this patch doesn't do that.
I was looking for XLogStandbyInfoActive()
It isn't there, so you're either breaking HS or missing a possible
optimisation. Having said that, it would be useful to be able to assume
that all xids appear in the log, for diagnostic purposes.
So I now agree with the way you've coded it.
--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services
On Sun, Dec 19, 2010 at 3:08 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
On Sun, 2010-12-19 at 07:33 -0500, Robert Haas wrote:
On Sun, Dec 19, 2010 at 7:01 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
On Fri, 2010-12-17 at 13:35 -0500, Robert Haas wrote:
I'm
thinking it makes sense to commit this part first.This will break Hot Standby, as previously explained. Don't.
Uh, why? Skipping the commit record altogether would do that, but
this patch doesn't do that.I was looking for XLogStandbyInfoActive()
It isn't there, so you're either breaking HS or missing a possible
optimisation. Having said that, it would be useful to be able to assume
that all xids appear in the log, for diagnostic purposes.So I now agree with the way you've coded it.
OK, thanks. Committed. Note that there is also a long comment in
there which includes a discussion of the issues relating to Hot
Standby. Hopefully that's clear enough to prevent anyone from getting
too clever with this in the future.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company