From d670765298ced11e5f147c319171306918b0285c Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Thu, 26 Feb 2015 12:50:07 +0100 Subject: [PATCH] Reconsider when to wait for WAL flushes/syncrep during commit. Up to now RecordTransactionCommit() waited for WAL to be flushed (if synchronous_commit != off) and to be synchronously replicated (if enabled), even if a transaction did not have a xid assigned. The primary reason for that is that sequence's nextval() did not assign a xid, but are worthwhile to wait for on commit. This can be problematic because sometimes read only transactions do write WAL, e.g. HOT page prune records. That then could lead to read only transactions having to wait during commit. Not something people expect in a read only transaction. This lead to such strange symptoms as backends being seemingly stuck during connection establishment when all synchronous replicas are down. Especially annoying when said stuck connection is the standby trying to reconnect to allow syncrep again... This behavior also is involved in a rather complicated <= 9.4 bug where the transaction started by catchup interrupt processing waited for syncrep using latches, but didn't get the wakeup because it was already running inside the same overloaded signal handler. Fix the issue here doesn't properly solve that issue, merely papers over the problems. In 9.5 catchup interrupts aren't processed out of signal handlers anymore. To fix all this, make nextval() acquire a top level xid, and only wait for transaction commit if a transaction both acquired a xid and emitted WAL records. If only a xid has been assigned we don't uselessly want to wait just because of writes to temporary/unlogged tables; if only WAL has been written we don't want to wait just because of HOT prunes. The xid assignment in nextval() is unlikely to cause overhead in real-world workloads. For one it only happens SEQ_LOG_VALS/32 values anyway, for another only usage of nextval() without using the result in an insert or similar is affected. Discussion: 20150223165359.GF30784@awork2.anarazel.de, 369698E947874884A77849D8FE3680C2@maumau, 5CF4ABBA67674088B3941894E22A0D25@maumau Per complaint from maumau and Thom Brown Backpatch all the way back; 9.0 doesn't have syncrep, but it seems better to be consistent behavior across all maintained branches. --- src/backend/access/transam/xact.c | 29 +++++++++++++++++------------ src/backend/commands/sequence.c | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index d5eb4a2687..749961d76f 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -994,10 +994,9 @@ RecordTransactionCommit(void) /* * If we didn't create XLOG entries, we're done here; otherwise we - * should flush those entries the same as a commit record. (An - * example of a possible record that wouldn't cause an XID to be - * assigned is a sequence advance record due to nextval() --- we want - * to flush that to disk before reporting commit.) + * should trigger flushing those entries the same as a commit record + * would. This will primarily happen for HOT pruning and the like; we + * want these to be flushed to disk in due time. */ if (!wrote_xlog) goto cleanup; @@ -1120,11 +1119,13 @@ RecordTransactionCommit(void) /* * 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 - * and/or unlogged tables. In case of a crash, the loss of such a - * transaction will be irrelevant since temp tables will be lost anyway, - * and unlogged tables will be truncated. (Given the foregoing, you might + * transaction has not performed any WAL-logged operation or didn't assign + * a xid. The transaction can end up not writing any WAL, even if it has + * a xid, if it only wrote to temporary and/or unlogged tables. It can + * end up having written WAL without an xid if it did HOT pruning. In + * case of a crash, the loss of such a transaction will be irrelevant; + * temp tables will be lost anyway, unlogged tables will be truncated and + * HOT pruning will be done again later. (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 @@ -1140,7 +1141,8 @@ RecordTransactionCommit(void) * if all to-be-deleted tables are temporary though, since they are lost * anyway if we crash.) */ - if ((wrote_xlog && synchronous_commit > SYNCHRONOUS_COMMIT_OFF) || + if ((wrote_xlog && markXidCommitted && + synchronous_commit > SYNCHRONOUS_COMMIT_OFF) || forceSyncCommit || nrels > 0) { /* @@ -1212,12 +1214,15 @@ RecordTransactionCommit(void) latestXid = TransactionIdLatest(xid, nchildren, children); /* - * Wait for synchronous replication, if required. + * Wait for synchronous replication, if required. Similar to the decision + * above about using committing asynchronously we only want to wait if + * this backend assigned a xid and wrote WAL. No need to wait if a xid + * was assigned due to temporary/unlogged tables or due to HOT pruning. * * Note that at this stage we have marked clog, but still show as running * in the procarray and continue to hold locks. */ - if (wrote_xlog) + if (wrote_xlog && markXidCommitted) SyncRepWaitForLSN(XactLastRecEnd); /* Reset XactLastRecEnd until the next transaction writes something */ diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c index dddb2e1a39..f4d7b9ae6c 100644 --- a/src/backend/commands/sequence.c +++ b/src/backend/commands/sequence.c @@ -15,6 +15,7 @@ #include "postgres.h" #include "access/transam.h" +#include "access/xact.h" #include "access/xlogutils.h" #include "catalog/dependency.h" #include "catalog/namespace.h" @@ -340,6 +341,10 @@ fill_seq_with_data(Relation rel, HeapTuple tuple) */ LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE); + /* check the comment above nextval_internal()'s equivalent call. */ + if (RelationNeedsWAL(rel)) + GetTopTransactionId(); + START_CRIT_SECTION(); { @@ -441,6 +446,10 @@ AlterSequence(AlterSeqStmt *stmt) /* Note that we do not change the currval() state */ elm->cached = elm->last; + /* check the comment above nextval_internal()'s equivalent call. */ + if (RelationNeedsWAL(seqrel)) + GetTopTransactionId(); + /* Now okay to update the on-disk tuple */ START_CRIT_SECTION(); @@ -683,6 +692,16 @@ nextval_internal(Oid relid) last_used_seq = elm; + /* + * If something needs to be WAL logged, acquire an xid, so this + * transaction's commit will trigger a WAL flush and wait for + * syncrep. It's sufficient to ensure the toplevel transaction has a xid, + * no need to assign xids subxacts, that'll already trigger a appropriate + * wait. (Have to do that here, so we're outside the critical section) + */ + if (logit && RelationNeedsWAL(seqrel)) + GetTopTransactionId(); + /* ready to change the on-disk (or really, in-buffer) tuple */ START_CRIT_SECTION(); @@ -877,6 +896,10 @@ do_setval(Oid relid, int64 next, bool iscalled) /* In any case, forget any future cached numbers */ elm->cached = elm->last; + /* check the comment above nextval_internal()'s equivalent call. */ + if (RelationNeedsWAL(seqrel)) + GetTopTransactionId(); + /* ready to change the on-disk (or really, in-buffer) tuple */ START_CRIT_SECTION(); -- 2.40.0