On Wed, 2009-11-25 at 13:00 +0200, Heikki Linnakangas wrote:
> I've put up a wiki page with the issues I see with the patch as it
> stands. They're roughly categorized by seriousness.
>
> http://wiki.postgresql.org/wiki/Hot_Standby_TODO
>
> New issues can and probably will still pop up, let's add them to the
> list as they're found so that we know what still needs to be done.
>
> You had a list of work items at the hot standby main page, but I believe
> it's badly out-of-date. Please move any still relevant items to the
> above list, if any.
4 changes on TODO included here, including all must-fix issues. This is
a combined patch. Will push changes to git also, so each commit is
visible separately.
Lots of wiki comments added.
--
Simon Riggs www.2ndQuadrant.com
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 75aca23..5af05fe 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1985,7 +1985,7 @@ if (!triggered)
</listitem>
<listitem>
<para>
- LOCK TABLE, though only when explicitly IN ACCESS SHARE MODE
+ LOCK TABLE, though only when explicitly in one of these modes: ACCESS SHARE, ROW SHARE or ROW EXCLUSIVE.
</para>
</listitem>
<listitem>
@@ -2033,7 +2033,7 @@ if (!triggered)
<listitem>
<para>
LOCK TABLE, in short default form, since it requests ACCESS EXCLUSIVE MODE.
- LOCK TABLE that explicitly requests a lock other than ACCESS SHARE MODE.
+ LOCK TABLE that explicitly requests a mode higher than ROW EXCLUSIVE MODE.
</para>
</listitem>
<listitem>
@@ -2110,10 +2110,10 @@ if (!triggered)
<para>
In recovery, transactions will not be permitted to take any table lock
- higher than AccessShareLock. In addition, transactions may never assign
+ higher than RowExclusiveLock. In addition, transactions may never assign
a TransactionId and may never write WAL.
Any LOCK TABLE command that runs on the standby and requests a specific
- lock type other than AccessShareLock will be rejected.
+ lock mode higher than ROW EXCLUSIVE MODE will be rejected.
</para>
<para>
@@ -2207,15 +2207,16 @@ if (!triggered)
<para>
We have a number of choices for resolving query conflicts. The default
- is that we wait and hope the query completes. The server will wait automatically until the lag between
- primary and standby is at most max_standby_delay seconds. Once that grace
- period expires, we take one of the following actions:
+ is that we wait and hope the query completes. The server will wait
+ automatically until the lag between primary and standby is at most
+ max_standby_delay seconds. Once that grace period expires, we take one
+ of the following actions:
<itemizedlist>
<listitem>
<para>
- If the conflict is caused by a lock, we cancel the standby transaction
- immediately, even if it is idle-in-transaction.
+ If the conflict is caused by a lock, we cancel the conflicting standby
+ transaction immediately, even if it is idle-in-transaction.
</para>
</listitem>
<listitem>
@@ -2224,7 +2225,9 @@ if (!triggered)
that a conflict has occurred and that it must cancel itself to avoid the
risk that it silently fails to read relevant data because
that data has been removed. (This is very similar to the much feared
- error message "snapshot too old").
+ error message "snapshot too old"). Some cleanup records only cause
+ conflict with older queries, though some types of cleanup record
+ affect all queries.
</para>
<para>
@@ -2364,6 +2367,9 @@ LOG: database system is ready to accept read only connections
<para>
As a result, you cannot create additional indexes that exist solely
on the standby, nor can statistics that exist solely on the standby.
+ If these administrator commands are needed they should be executed
+ on the primary so that the changes will propagate through to the
+ standby.
</para>
<para>
@@ -2373,7 +2379,8 @@ LOG: database system is ready to accept read only connections
managed by the Startup process that owns all AccessExclusiveLocks held
by transactions being replayed by recovery. pg_stat_activity does not
show an entry for the Startup process, nor do recovering transactions
- show as active.
+ show as active. Note that Startup process does not acquire locks to
+ make database changes and thus no locks are shown in pg_locks.
</para>
<para>
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 364756d..05e1c5e 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -659,7 +659,10 @@ _bt_page_recyclable(Page page)
* order when replaying the effects of a VACUUM, just as we do for the
* original VACUUM itself. lastBlockVacuumed allows us to tell whether an
* intermediate range of blocks has had no changes at all by VACUUM,
- * and so must be scanned anyway during replay.
+ * and so must be scanned anyway during replay. We always write a WAL record
+ * for the last block in the index, whether or not it contained any items
+ * to be removed. This allows us to scan right up to end of index to
+ * ensure correct locking.
*/
void
_bt_delitems(Relation rel, Buffer buf,
@@ -675,7 +678,8 @@ _bt_delitems(Relation rel, Buffer buf,
START_CRIT_SECTION();
/* Fix the page */
- PageIndexMultiDelete(page, itemnos, nitems);
+ if (nitems > 0)
+ PageIndexMultiDelete(page, itemnos, nitems);
/*
* We can clear the vacuum cycle ID since this page has certainly been
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 525d838..5c28e55 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -678,7 +678,34 @@ btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
/* Quit if we've scanned the whole relation */
if (blkno >= num_pages)
+ {
+ /*
+ * InHotStandby we need to scan right up to the end of the index for
+ * correct locking, so we may need to write a WAL record for the final
+ * block in the index if it was not vacuumed. It's possible that
+ * VACUUMing has actually removed zeroed pages at the end of the index
+ * so we need to take care to issue the record for last actual block and not
+ * for the last block that was scanned. Ignore empty indexes.
+ */
+ if (num_pages > 1 && vstate.lastBlockVacuumed < (num_pages - 1))
+ {
+ Buffer buf;
+
+ /*
+ * We can't use _bt_getbuf() here because it always applies
+ * _bt_checkpage(), which will barf on an all-zero page. We want to
+ * recycle all-zero pages, not fail. Also, we want to use a nondefault
+ * buffer access strategy.
+ */
+ buf = ReadBufferExtended(rel, MAIN_FORKNUM, num_pages - 1, RBM_NORMAL,
+ info->strategy);
+ LockBufferForCleanup(buf);
+ _bt_delitems(rel, buf, NULL, 0, true, vstate.lastBlockVacuumed);
+ _bt_relbuf(rel, buf);
+ }
break;
+ }
+
/* Iterate over pages, then loop back to recheck length */
for (; blkno < num_pages; blkno++)
{
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index f66979d..533b218 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -536,7 +536,8 @@ btree_xlog_vacuum(XLogRecPtr lsn, XLogRecord *record)
unused = (OffsetNumber *) ((char *) xlrec + SizeOfBtreeVacuum);
unend = (OffsetNumber *) ((char *) xlrec + record->xl_len);
- PageIndexMultiDelete(page, unused, unend - unused);
+ if ((unend - unused) > 0)
+ PageIndexMultiDelete(page, unused, unend - unused);
}
/*
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e209d46..1a9a20b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -129,11 +129,12 @@ TimeLineID ThisTimeLineID = 0;
* recovery mode". It should be examined primarily by functions that need
* to act differently when called from a WAL redo function (e.g., to skip WAL
* logging). To check whether the system is in recovery regardless of which
- * process you're running in, use RecoveryInProgress().
+ * process you're running in, use RecoveryInProgress() but only after shared
+ * memory startup and lock initialization.
*/
bool InRecovery = false;
-/* Are we in Hot Standby mode? */
+/* Are we in Hot Standby mode? Only valid in startup process, see xlog.h */
HotStandbyState standbyState = STANDBY_DISABLED;
static XLogRecPtr LastRec;
@@ -5204,22 +5205,43 @@ recoveryStopsHere(XLogRecord *record, bool *includeThis)
TimestampTz recordXtime;
/* We only consider stopping at COMMIT or ABORT records */
- if (record->xl_rmid != RM_XACT_ID)
- return false;
- record_info = record->xl_info & ~XLR_INFO_MASK;
- if (record_info == XLOG_XACT_COMMIT)
+ if (record->xl_rmid == RM_XACT_ID)
{
- xl_xact_commit *recordXactCommitData;
+ record_info = record->xl_info & ~XLR_INFO_MASK;
+ if (record_info == XLOG_XACT_COMMIT)
+ {
+ xl_xact_commit *recordXactCommitData;
- recordXactCommitData = (xl_xact_commit *) XLogRecGetData(record);
- recordXtime = recordXactCommitData->xact_time;
+ recordXactCommitData = (xl_xact_commit *) XLogRecGetData(record);
+ recordXtime = recordXactCommitData->xact_time;
+ }
+ else if (record_info == XLOG_XACT_ABORT)
+ {
+ xl_xact_abort *recordXactAbortData;
+
+ recordXactAbortData = (xl_xact_abort *) XLogRecGetData(record);
+ recordXtime = recordXactAbortData->xact_time;
+ }
+ else
+ return false;
}
- else if (record_info == XLOG_XACT_ABORT)
+ else if (record->xl_rmid == RM_XLOG_ID)
{
- xl_xact_abort *recordXactAbortData;
+ record_info = record->xl_info & ~XLR_INFO_MASK;
+ if (record_info == XLOG_CHECKPOINT_SHUTDOWN ||
+ record_info == XLOG_CHECKPOINT_ONLINE)
+ {
+ CheckPoint checkPoint;
+
+ memcpy(&checkPoint, XLogRecGetData(record), sizeof(CheckPoint));
+ recoveryLastXTime = checkPoint.time;
+ }
- recordXactAbortData = (xl_xact_abort *) XLogRecGetData(record);
- recordXtime = recordXactAbortData->xact_time;
+ /*
+ * We don't want to stop recovery on a checkpoint record, but we do
+ * want to update recoveryLastXTime. So return is unconditional.
+ */
+ return false;
}
else
return false;
@@ -7188,14 +7210,9 @@ CreateRestartPoint(int flags)
LogCheckpointEnd(true);
ereport((log_checkpoints ? LOG : DEBUG2),
- (errmsg("recovery restart point at %X/%X",
- lastCheckPoint.redo.xlogid, lastCheckPoint.redo.xrecoff)));
-
- /* XXX this is currently BROKEN because we are in the wrong process */
- if (recoveryLastXTime)
- ereport((log_checkpoints ? LOG : DEBUG2),
- (errmsg("last completed transaction was at log time %s",
- timestamptz_to_str(recoveryLastXTime))));
+ (errmsg("recovery restart point at %X/%X with latest known log time %s",
+ lastCheckPoint.redo.xlogid, lastCheckPoint.redo.xrecoff,
+ timestamptz_to_str(GetLatestXLogTime()))));
LWLockRelease(CheckpointLock);
return true;
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index e8cc15f..f4a97b1 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -49,9 +49,12 @@ LockTableCommand(LockStmt *lockstmt)
/*
* During recovery we only accept these variations:
- * LOCK TABLE foo IN ACCESS SHARE MODE which is effectively a no-op
+ * LOCK TABLE foo IN ACCESS SHARE MODE
+ * LOCK TABLE foo IN ROW SHARE MODE
+ * LOCK TABLE foo IN ROW EXCLUSIVE MODE
+ * This test must match the restrictions defined in LockAcquire()
*/
- if (lockstmt->mode != AccessShareLock)
+ if (lockstmt->mode > RowExclusiveLock)
PreventCommandDuringRecovery();
LockTableRecurse(reloid, relation,
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 869c7a6..4005c01 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1228,9 +1228,7 @@ GetRunningTransactionData(void)
int index;
int count;
int subcount;
- int topcount;
bool suboverflowed;
- XLogRecPtr recptr;
Assert(!RecoveryInProgress());
@@ -1259,7 +1257,7 @@ GetRunningTransactionData(void)
xids = CurrentRunningXacts->xids;
- count = subcount = topcount = 0;
+ count = subcount = 0;
suboverflowed = false;
/*
@@ -1292,7 +1290,6 @@ GetRunningTransactionData(void)
continue;
xids[count++] = xid;
- topcount++;
if (!TransactionIdIsValid(oldestRunningXid) ||
TransactionIdPrecedes(xid, oldestRunningXid))
@@ -1329,18 +1326,6 @@ GetRunningTransactionData(void)
LWLockRelease(XidGenLock);
LWLockRelease(ProcArrayLock);
-
- if (CurrentRunningXacts->subxid_overflow)
- ereport(trace_recovery(DEBUG2),
- (errmsg("snapshot of %u running transactions overflowed (lsn %X/%X)",
- topcount,
- recptr.xlogid, recptr.xrecoff)));
- else
- ereport(trace_recovery(DEBUG2),
- (errmsg("snapshot of %u running transactions with %u subtransactions (lsn %X/%X)",
- topcount, subcount,
- recptr.xlogid, recptr.xrecoff)));
-
return CurrentRunningXacts;
}
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 8f1a6bc..561ccd6 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -319,6 +319,7 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
/*
* Wait for lock to clear or kill anyone in our way.
*/
+ ReportLockTableError(false);
while (LockAcquire(&locktag, AccessExclusiveLock, true, true)
== LOCKACQUIRE_NOT_AVAIL)
{
@@ -328,6 +329,8 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
ResolveRecoveryConflictWithVirtualXIDs(old_lockholders,
"exclusive lock",
CONFLICT_MODE_ERROR);
+ ReportLockTableError(true);
+
/*
* It's possible that other users may have aquired the lock while
* we were waiting for the first group of users. So we loop.
@@ -339,6 +342,7 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
* will endlessly spring alive again and re-request the same lock.
*/
}
+ ReportLockTableError(true);
}
static void
@@ -578,6 +582,7 @@ LogCurrentRunningXacts(RunningTransactions CurrRunningXacts)
xl_running_xacts xlrec;
XLogRecData rdata[2];
int lastrdata = 0;
+ XLogRecPtr recptr;
xlrec.xcnt = CurrRunningXacts->xcnt;
xlrec.subxid_overflow = CurrRunningXacts->subxid_overflow;
@@ -601,7 +606,19 @@ LogCurrentRunningXacts(RunningTransactions CurrRunningXacts)
rdata[lastrdata].next = NULL;
- XLogInsert(RM_STANDBY_ID, XLOG_RUNNING_XACTS, rdata);
+ recptr = XLogInsert(RM_STANDBY_ID, XLOG_RUNNING_XACTS, rdata);
+
+ if (CurrRunningXacts->subxid_overflow)
+ ereport(trace_recovery(DEBUG2),
+ (errmsg("snapshot of %u running transactions overflowed (lsn %X/%X)",
+ CurrRunningXacts->xcnt,
+ recptr.xlogid, recptr.xrecoff)));
+ else
+ ereport(trace_recovery(DEBUG2),
+ (errmsg("snapshot of %u running transaction ids (lsn %X/%X)",
+ CurrRunningXacts->xcnt,
+ recptr.xlogid, recptr.xrecoff)));
+
}
static void
diff --git a/src/backend/storage/lmgr/README b/src/backend/storage/lmgr/README
index 10f0be4..5cb2a2e 100644
--- a/src/backend/storage/lmgr/README
+++ b/src/backend/storage/lmgr/README
@@ -517,3 +517,28 @@ interfere with each other.
User locks are always held as session locks, so that they are not released at
transaction end. They must be released explicitly by the application --- but
they are released automatically when a backend terminates.
+
+Locking during Hot Standby
+--------------------------
+
+The Startup process is the only backend that can make changes during
+recovery, all other backends are read only. As a result the Startup
+process does not acquire locks on relations or objects except when the lock
+level is AccessExclusiveLock.
+
+Regular backends are only allowed to take locks on relations or objects
+at RowExclusiveLock or lower. This ensures that they do not conflict with
+each other or with the Startup process, unless AccessExclusiveLocks are
+requested by one of the backends.
+
+Deadlocks involving AccessExclusiveLocks are not possible, so we need
+not be concerned that a user initiated deadlock can prevent recovery from
+progressing.
+
+AccessExclusiveLocks on the primary or master node generate WAL records
+that are then applied by the Startup process. Locks are released at end
+of transaction just as they are in normal processing. These locks are
+held by the Startup process, acting as a proxy for the backends that
+originally acquired these locks. Again, these locks cannot conflict with
+one another, so the Startup process cannot deadlock itself either.
+
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index fc1ae1d..c7d2325 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -173,6 +173,7 @@ static HTAB *LockMethodLocalHash;
static LOCALLOCK *awaitedLock;
static ResourceOwner awaitedOwner;
+static bool reportLockTableError = true;
#ifdef LOCK_DEBUG
@@ -492,13 +493,14 @@ LockAcquire(const LOCKTAG *locktag,
elog(ERROR, "unrecognized lock mode: %d", lockmode);
if (RecoveryInProgress() &&
- locktag->locktag_type == LOCKTAG_OBJECT &&
- lockmode > AccessShareLock)
+ (locktag->locktag_type == LOCKTAG_OBJECT ||
+ locktag->locktag_type == LOCKTAG_RELATION ) &&
+ lockmode > RowExclusiveLock)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("cannot acquire lockmode %s on database objects while recovery is in progress",
lockMethodTable->lockModeNames[lockmode]),
- errhint("Only AccessShareLock can be acquired on database objects during recovery.")));
+ errhint("Only RowExclusiveLock or less can be acquired on database objects during recovery.")));
#ifdef LOCK_DEBUG
if (LOCK_DEBUG_ENABLED(locktag))
@@ -654,10 +656,13 @@ LockAcquire(const LOCKTAG *locktag,
elog(PANIC, "lock table corrupted");
}
LWLockRelease(partitionLock);
- ereport(ERROR,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of shared memory"),
- errhint("You might need to increase max_locks_per_transaction.")));
+ if (reportLockTableError)
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of shared memory"),
+ errhint("You might need to increase max_locks_per_transaction.")));
+ else
+ return LOCKACQUIRE_NOT_AVAIL;
}
locallock->proclock = proclock;
@@ -861,6 +866,14 @@ LockAcquire(const LOCKTAG *locktag,
return LOCKACQUIRE_OK;
}
+void
+ReportLockTableError(bool report)
+{
+ Assert(InHotStandby);
+
+ reportLockTableError = report;
+}
+
/*
* Subroutine to free a locallock entry
*/
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index e0b0672..b1ac641 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -495,6 +495,13 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
InitBufferPoolBackend();
/*
+ * Initialize local process's access to XLOG. In bootstrap case we may
+ * skip this since StartupXLOG() was run instead.
+ */
+ if (!bootstrap)
+ InitXLOGAccess();
+
+ /*
* Initialize the relation cache and the system catalog caches. Note that
* no catalog access happens here; we only set up the hashtable structure.
* We must do this before starting a transaction because transaction abort
@@ -615,19 +622,10 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
* way of declaring a session as being guaranteed-read-only, we could use
* AccessShareLock for such sessions and thereby not conflict against
* CREATE DATABASE.
- *
- * In hot standby mode, we use AccessShareLock because taking higher grade
- * locks is not allowed during recovery. CREATE DATABASE waits for any
- * connected backends to die, regardless of the lock level so this should
- * be fine. We normally take a RowExclusiveLock to make sure we block
- * and wait for any CREATE DATABASE that's running to finish, but no
- * CREATE DATABASE can be running during recovery. If a CREATE DATABASE
- * command is just being replayed from WAL, that's fine too because this
- * backend will be read-only until the replay is finished.
*/
if (!bootstrap)
LockSharedObject(DatabaseRelationId, MyDatabaseId, 0,
- (RecoveryInProgress() ? AccessShareLock : RowExclusiveLock));
+ RowExclusiveLock);
/*
* Recheck pg_database to make sure the target database hasn't gone away.
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index c27f734..35d21c5 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -153,8 +153,8 @@ extern bool InRecovery;
* checkpoint record to initialize our master transaction tracking system.
*
* When the transaction tracking is initialized, we enter the SNAPSHOT_PENDING
- * state. The tracked information might still be incomplete, so we can't let
- * backens in yet, but redo functions need to update the in-memory state when
+ * state. The tracked information might still be incomplete, so we can't allow
+ * connections yet, but redo functions must update the in-memory state when
* appropriate.
*
* In SNAPSHOT_READY mode, we have full knowledge of transactions that are
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index fd3b305..351cb05 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -495,6 +495,8 @@ extern void RemoveFromWaitQueue(PGPROC *proc, uint32 hashcode);
extern Size LockShmemSize(void);
extern LockData *GetLockStatusData(void);
+extern void ReportLockTableError(bool report);
+
typedef struct xl_standby_lock
{
TransactionId xid; /* xid of holder of AccessExclusiveLock */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
