Re: Hot Standby 0.2.1

Simon Riggs wrote:
> OK, here is the latest version of the Hot Standby patchset. This is
> about version 30+ by now, but we should regard this as 0.2.1
> Patch against CVS HEAD (now): clean apply, compile, no known bugs.

Thanks! Attached is some minor comment and fixes, and some dead code
removal. Also available in my git repository, branch 'hs-riggs'.

The documentation talks about setting and checking
default_transaction_read_only, but I think it doesn't say anything about
transaction_read_only, which I find odd. This in particular:

> Users will be able to tell whether their session is read-only by
> +   issuing SHOW default_transaction_read_only

seems misleading, as you might have default_transaction_read_only=on,
but still be able to do "SET transaction_read_only", so the *session*
isn't necessarily read-only.

The only bug I've found is this that we seem to be missing conflict
resolution for GiST index tuples deleted by the kill_prior_tuples
mechanism. Unless I'm missing something, we need similar handling there
that we have in b-tree.

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 91917cf..2257ec6 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -2099,9 +2099,9 @@ if (!triggered)

    <para>
     In recovery, transactions will not be permitted to take any lock higher
-    other than AccessShareLock or AccessExclusiveLock. In addition,
-    transactions may never assign a TransactionId and may never write WAL.
-    The LOCK TABLE command by default applies an AccessExclusiveLock.
+    than AccessShareLock. In addition, transactions may never assign a
+        TransactionId and may never write WAL.
+    The LOCK TABLE command by default applies an AccessExclusiveLock.
     Any LOCK TABLE command that runs on the standby and requests a specific
     lock type other than AccessShareLock will be rejected.
    </para>
@@ -2168,8 +2168,8 @@ if (!triggered)

    <para>
     An example of the above would be an Administrator on Primary server
-    runs a DROP TABLE command that refers to a table currently in use by
-    a User query on the standby server.
+    runs a DROP TABLE command on a table that's currently being queried
+    in the standby server.
    </para>

    <para>
@@ -2198,9 +2198,9 @@ if (!triggered)
    <para>
     We have a number of choices for resolving query conflicts.  The default
     is that we wait and hope the query completes. If the recovery is not paused,
-    then the server will wait automatically until the server the lag between
+    then 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 then take one of the following actions:
+    period expires, we take one of the following actions:

       <itemizedlist>
        <listitem>
@@ -2213,7 +2213,7 @@ if (!triggered)
         <para>
          If the conflict is caused by cleanup records we tell the standby query
           that a conflict has occurred and that it must cancel itself to avoid the
-          risk that it attempts to silently fails to read relevant data because
+         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").
         </para>
@@ -2222,7 +2222,7 @@ if (!triggered)
          Note also that this means that idle-in-transaction sessions are never
          canceled except by locks. Users should be clear that tables that are
          regularly and heavily updated on primary server will quickly cause
-         cancellation of any longer running queries made against those tables.
+         cancellation of any longer running queries in the standby.
         </para>

         <para>
@@ -2235,7 +2235,7 @@ if (!triggered)
    </para>

    <para>
-    Other remdial actions exist if the number of cancelations is unacceptable.
+    Other remedial actions exist if the number of cancelations is unacceptable.
     The first option is to connect to primary server and keep a query active
     for as long as we need to run queries on the standby. This guarantees that
     a WAL cleanup record is never generated and we don't ever get query
@@ -2283,7 +2283,7 @@ if (!triggered)
    <title>Administrator's Overview</title>

    <para>
-    If there is a recovery.conf file present then the will start in Hot Standby
+    If there is a recovery.conf file present the server will start in Hot Standby
     mode by default, though this can be disabled by setting
     "recovery_connections = off" in recovery.conf. The server may take some
     time to enable recovery connections since the server must first complete
@@ -2329,7 +2329,7 @@ LOG:  database system is ready to accept read only connections
     A set of functions allow superusers to control the flow of recovery
     are described in <xref linkend="functions-recovery-control-table">.
     These functions allow you to pause and continue recovery, as well
-    as dynamically set new recovery targets wile recovery progresses.
+    as dynamically set new recovery targets while recovery progresses.
     Note that when a server is paused the apparent delay between primary
     and standby will continue to increase.
    </para>
@@ -2354,7 +2354,7 @@ LOG:  database system is ready to accept read only connections
    </para>

    <para>
-    The following types of administrator command will not be accepted
+    The following types of administrator command are not accepted
     during recovery mode

       <itemizedlist>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 06243c0..b38b344 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -372,9 +372,9 @@ SET ENABLE_SEQSCAN TO OFF;
        </para>

        <para>
-        When running a standby server, it is strongly recommended that you
-        set this parameter to be the same or higher than the master server.
-        Otherwise, queries on the standby server may fail.
+        When running a standby server, you must set this parameter to the
+        same or higher value than on the master server. Otherwise, queries on
+        will not be allowed in the standby server.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/backend/access/gin/ginxlog.c b/src/backend/access/gin/ginxlog.c
index 94b7202..c6612b8 100644
--- a/src/backend/access/gin/ginxlog.c
+++ b/src/backend/access/gin/ginxlog.c
@@ -622,7 +622,7 @@ gin_redo(XLogRecPtr lsn, XLogRecord *record)
     uint8        info = record->xl_info & ~XLR_INFO_MASK;

     /*
-     * GIN indexes do not require any conflict processing. XXX really?
+     * GIN indexes do not require any conflict processing.
      */
     if (InHotStandby)
         RecordKnownAssignedTransactionIds(record->xl_xid);
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index 3e5f3b6..8ed49e0 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -397,7 +397,8 @@ gist_redo(XLogRecPtr lsn, XLogRecord *record)
     MemoryContext oldCxt;

     /*
-     * GIST indexes do not require any conflict processing. XXX really?
+     * GIST indexes do not require any conflict processing. XXX what about
+     * killed tuples?
      */
     if (InHotStandby)
         RecordKnownAssignedTransactionIds(record->xl_xid);
diff --git a/src/backend/access/nbtree/README b/src/backend/access/nbtree/README
index c58d2fc..d7dfbdd 100644
--- a/src/backend/access/nbtree/README
+++ b/src/backend/access/nbtree/README
@@ -410,7 +410,7 @@ situation the locking requirements can be relaxed and we do not need
 double locking during block splits. Each WAL record makes changes to a
 single level of the btree using the correct locking sequence and so
 is safe for concurrent readers. Some readers may observe a block split
-in progress as they descend the tree, but they will simple move right
+in progress as they descend the tree, but they will simpye move right
 onto the correct page.

 During recovery all index scans start with ignore_killed_tuples = false
@@ -419,8 +419,8 @@ on the standby server can be older than the oldest xmin on the master
 server, which means tuples can be marked as killed even when they are
 still visible on the standby. We don't WAL log tuple killed bits, but
 they can still appear in the standby because of full page writes. So
-we must always ignore them and that means it's not worth setting them
-either.
+we must always ignore them in standby, and that means it's not worth
+setting them either.

 Other Things That Are Handy to Know
 -----------------------------------
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index eefa888..7871524 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -721,6 +721,7 @@ _bt_delitems(Relation rel, Buffer buf,
              * We would like to set an accurate latestRemovedXid, but there
              * is no easy way of obtaining a useful value. So we use the
              * probably far too conservative value of RecentGlobalXmin instead.
+             * XXX: this comment is bogus? We don't use RecentGlobalXmin
              */
             xlrec_delete.latestRemovedXid = InvalidTransactionId;
             rdata[0].data = (char *) &xlrec_delete;
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index 864c41c..6b962b6 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -23,7 +23,7 @@
 #include "miscadmin.h"

 /*
- * We must keep track of expected insertions due to page spl    its, and apply
+ * We must keep track of expected insertions due to page splits, and apply
  * them manually if they are not seen in the WAL log during replay.  This
  * makes it safe for page insertion to be a multiple-WAL-action process.
  *
@@ -503,8 +503,8 @@ btree_xlog_vacuum(XLogRecPtr lsn, XLogRecord *record)
     }

     /*
-     * We need to take a cleanup lock to apply these changes.
-     * See nbtree/README for details.
+     * Like in btvacuumpage(), we need to take a cleanup lock on every leaf
+     * page. See nbtree/README for details.
      */
     buffer = XLogReadBufferExtended(xlrec->node, MAIN_FORKNUM, xlrec->block, RBM_NORMAL);
     if (!BufferIsValid(buffer))
@@ -805,11 +805,11 @@ btree_redo(XLogRecPtr lsn, XLogRecord *record)

     /*
      * Btree delete records can conflict with standby queries. You might
-     * think that vacuum records would conflict as well, but they don't.
-     * XLOG_HEAP2_CLEANUP_INFO records provide the highest xid cleaned
-     * by the vacuum of the heap and so we can resolve any conflicts just
-     * once when that arrives. After that any we know that no conflicts exist
-     * from individual btree vacuum records on that index.
+     * think that vacuum records would conflict as well, but we've handled
+     * that already. XLOG_HEAP2_CLEANUP_INFO records provide the highest xid
+     * cleaned by the vacuum of the heap and so we can resolve any conflicts
+     * just once when that arrives. After that any we know that no conflicts
+     * exist from individual btree vacuum records on that index.
      */
     if (InHotStandby)
     {
diff --git a/src/backend/access/transam/README b/src/backend/access/transam/README
index fc7ecfd..46b48f0 100644
--- a/src/backend/access/transam/README
+++ b/src/backend/access/transam/README
@@ -195,8 +195,7 @@ they first do something that requires one --- typically, insert/update/delete
 a tuple, though there are a few other places that need an XID assigned.
 If a subtransaction requires an XID, we always first assign one to its
 parent.  This maintains the invariant that child transactions have XIDs later
-than their parents, which is assumed in a number of places. In 8.5 onwards,
-some corner cases exist that require XID assignment to be WAL logged.
+than their parents, which is assumed in a number of places.

 The subsidiary actions of obtaining a lock on the XID and and entering it into
 pg_subtrans and PG_PROC are done at the time it is assigned.
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 28d1cf0..84c0d54 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -376,7 +376,6 @@ ProcArrayClearTransaction(PGPROC *proc)
     proc->lxid = InvalidLocalTransactionId;
     proc->xmin = InvalidTransactionId;
     proc->recoveryConflictMode = 0;
-    proc->recoveryConflictLSN = InvalidXLogRecPtr;

     /* redundant, but just in case */
     proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
@@ -1171,7 +1170,7 @@ void
 GetRunningTransactionData(void)
 {
     ProcArrayStruct *arrayP = procArray;
-    static RunningTransactions CurrentRunningXacts = (RunningTransactions) &CurrentRunningXactsData;
+    RunningTransactions CurrentRunningXacts = (RunningTransactions) &CurrentRunningXactsData;
     RunningXact    *rxact;
     TransactionId *subxip;
     TransactionId latestRunningXid = InvalidTransactionId;
@@ -1274,14 +1273,8 @@ GetRunningTransactionData(void)
         numHeldLocks += proc->numHeldLocks;

         /*
-         * Save subtransaction XIDs.
-         *
-         * The other backend can add more subxids concurrently, but cannot
-         * remove any.    Hence it's important to fetch nxids just once. Should
-         * be safe to use memcpy, though.  (We needn't worry about missing any
-         * xids added concurrently, because they must postdate xmax.)
-         *
-         * Again, our own XIDs *are* included in the snapshot.
+         * Save subtransaction XIDs. Other backends can't add or remove entries
+         * while we're holding XidGenLock.
          */
         nxids = proc->subxids.nxids;

@@ -1536,41 +1529,6 @@ BackendPidGetProc(int pid)
 }

 /*
- * BackendXidGetProc -- get a backend's PGPROC given its XID
- *
- * Returns NULL if not found.  Note that it is up to the caller to be
- * sure that the question remains meaningful for long enough for the
- * answer to be used ...
- */
-PGPROC *
-BackendXidGetProc(TransactionId xid)
-{
-    PGPROC       *result = NULL;
-    ProcArrayStruct *arrayP = procArray;
-    int            index;
-
-    if (xid == InvalidTransactionId)    /* never match invalid xid */
-        return 0;
-
-    LWLockAcquire(ProcArrayLock, LW_SHARED);
-
-    for (index = 0; index < arrayP->numProcs; index++)
-    {
-        PGPROC       *proc = arrayP->procs[index];
-
-        if (proc->xid == xid)
-        {
-            result = proc;
-            break;
-        }
-    }
-
-    LWLockRelease(ProcArrayLock);
-
-    return result;
-}
-
-/*
  * BackendXidGetPid -- get a backend's pid given its XID
  *
  * Returns 0 if not found or it's a prepared transaction.  Note that
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index d05e7a2..48d6553 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -1586,15 +1586,11 @@ ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist,
                 /*
                  * Issue orders for the proc to read next time it receives SIGINT
                  * Note that this is an atomic change and requires no locking.
+                 *
+                 * XXX: Huh?
                  */
                 if (proc->recoveryConflictMode < cancel_mode)
-                {
-                    if (cancel_mode == ERROR &&
-                        XLByteLT(proc->recoveryConflictLSN, conflict_lsn))
-                        proc->recoveryConflictLSN = conflict_lsn;
-
                     proc->recoveryConflictMode = cancel_mode;
-                }

                 /*
                  * Do we expect it to talk? No, Mr. Bond, we expect it to die.
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index bfcb75c..91b4e23 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -536,10 +536,6 @@ typedef BTScanOpaqueData *BTScanOpaque;
 #define SK_BT_DESC            (INDOPTION_DESC << SK_BT_INDOPTION_SHIFT)
 #define SK_BT_NULLS_FIRST    (INDOPTION_NULLS_FIRST << SK_BT_INDOPTION_SHIFT)

-/* XXX probably needs new RMgr call to do this cleanly */
-extern bool btree_is_cleanup_record(uint8 info);
-extern bool btree_needs_cleanup_lock(uint8 info);
-
 /*
  * prototypes for functions in nbtree.c (external entry points for btree)
  */
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index f674547..895532d 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -294,7 +294,6 @@ typedef struct LOCKTAG
  * nRequested -- total requested locks of all types.
  * granted -- count of each lock type currently granted on the lock.
  * nGranted -- total granted locks of all types.
- * xid -- xid of current/only lock holder for use by GetLockStatusData()
  *
  * Note: these counts count 1 for each backend.  Internally to a backend,
  * there may be multiple grabs on a particular lock, but this is not reflected
@@ -375,6 +374,11 @@ typedef struct PROCLOCK
 #define PROCLOCK_LOCKMETHOD(proclock) \
     LOCK_LOCKMETHOD(*((proclock).tag.myLock))

+/*
+ * Does acquisition of this lock need to be replayed in a standby server?
+ * Only AccessExclusiveLocks can conflict with lock types that read-only
+ * transactions can acquire in a standby server.
+ */
 #define IsProcLockLoggable(proclock)    \
             ((proclock->holdMask & LOCKBIT_ON(AccessExclusiveLock)) && \
              (proclock->tag.myLock)->tag.locktag_type == LOCKTAG_RELATION)
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 5729482..0df2529 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -99,12 +99,9 @@ struct PGPROC
     int            numHeldLocks;    /* Number of AccessExclusiveLocks held by
                                  * current backend. */
     /*
-     * InHotStandby mode, the lsn of the first conflict, if any.
-     * Any block seen with changes made after this lsn will trigger
-     * query cancelation. Always set recoveryConflictCancelMode after
-     * setting conflictLSN so we can check this without spin locking.
+     * While in hot standby mode, setting recoveryConflictMode instructs
+     * the backend to commit suicide.
      */
-    XLogRecPtr     recoveryConflictLSN;
     int            recoveryConflictMode;

     /* Info about LWLock the process is currently waiting for, if any. */
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index b882795..36d5d52 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -54,7 +54,6 @@ extern int    GetTransactionsInCommit(TransactionId **xids_p);
 extern bool HaveTransactionsInCommit(TransactionId *xids, int nxids);

 extern PGPROC *BackendPidGetProc(int pid);
-extern PGPROC *BackendXidGetProc(TransactionId xid);
 extern int    BackendXidGetPid(TransactionId xid);
 extern bool IsBackendPid(int pid);

diff --git a/src/include/utils/snapshot.h b/src/include/utils/snapshot.h
index 92feaf8..67e8112 100644
--- a/src/include/utils/snapshot.h
+++ b/src/include/utils/snapshot.h
@@ -17,6 +17,7 @@
 #include "storage/buf.h"
 #include "storage/sinval.h"

+
 typedef struct SnapshotData *Snapshot;

 #define InvalidSnapshot        ((Snapshot) NULL)
@@ -51,7 +52,8 @@ typedef struct SnapshotData
     /* note: all ids in xip[] satisfy xmin <= xip[i] < xmax */
     int32        subxcnt;        /* # of xact ids in subxip[], -1 if overflow */
     TransactionId *subxip;        /* array of subxact IDs in progress */
-    bool        takenDuringRecovery;    /* Recovery-shaped snapshot? */
+    bool        takenDuringRecovery;    /* recovery-shaped snapshot? */
+
     /*
      * note: all ids in subxip[] are >= xmin, but we don't bother filtering
      * out any that are >= xmax