Hot Standby (commit fest version - v5)

Hot Standby patch, including all major planned features.

Allows users to connect to server in archive recovery and run queries.

Applies cleanly, passes make check.

There's no flaky code, kludges or other last minute rush-for-deadline
stuff. "It all works", though really for a patch this size and scope I
expect many bugs. As a result I am describing this as "WIP", though it
is much more than a prototype. All the code has been planned out in
advance, so there's been very little on-the-fly changes required during
final development. I'm fully committed to making all required changes
and fixes in reasonable times. I will continue detailed testing over the
next few weeks to re-check everything prior to commit.

Initially, I would ask people to spend time thinking about this
conceptually to check that I have all the correct subsystems and cater
for all the little side tweaks that exist in various parts of the server

When you test this, please do it on a server built with --enable-cassert
and keep a detailed log using trace_recovery_messages = DEBUG4 (or 2-3).

Code has been specifically designed to be performance neutral or better
for normal workloads, so the WAL volume, number of times we take WAL
locks etc should be identical (on 64-bit systems). The patch includes
further tuning of subtransaction commits, so I hope the patch may even
be a few % win on more complex workloads with many PL/pgSQL functions
using EXCEPTION. Enabling the bgwriter during recovery seems likely to
be a huge gain on performance for larger shared_buffers settings, which
should offset a considerable increase in CPU usage during recovery.

Performance test results would be appreciated
* for normal running - just to test that it really is neutral
* for recovery - if query access not used is it faster than before

Points of significant note for detailed reviewers

* Prepared transactions not implemented yet. No problems foreseen, but
want to wait to see if other refactorings are required.

* Touching every block of a btree index during a replay of VACUUM is not
yet implemented. Would like some second opinions that it is even
required. I have code prototyped for it, but it feels too wacky even
after Heikki and I agreed it was required. Do we still think that?

* locking correctness around flat file refresh still not enabled

* need some discussiona round how to handle max_connections changes
cleanly.

Overview of code changes
========================

Docs
doc/src/sgml/config.sgml | 26
doc/src/sgml/func.sgml | 191 +++
src/backend/storage/buffer/README | 9

Transaction emulation in recovery
src/backend/access/transam/xact.c | 1022 ++++++++++++++++++-
src/backend/storage/ipc/procarray.c | 868 +++++++++++++++!
src/backend/utils/time/tqual.c | 62 !
src/backend/access/heap/pruneheap.c | 18
src/backend/access/transam/clog.c | 3
src/backend/access/transam/multixact.c | 14
src/backend/access/transam/slru.c | 16
src/backend/access/transam/subtrans.c | 27
src/backend/access/transam/twophase.c | 4
src/backend/access/transam/xlogutils.c | 18
src/backend/storage/lmgr/proc.c | 110 ++
src/include/storage/proc.h | 15
src/include/storage/procarray.h | 29
src/include/access/heapam.h | 4
src/include/access/htup.h | 18
src/include/access/xact.h | 56 +
src/include/access/xlog.h | 54 !
src/include/access/xlog_internal.h | 16
src/include/access/xlogutils.h | 3
src/include/storage/bufmgr.h | 7
src/include/utils/snapshot.h | 82 +

Locking and invalidation in recovery
src/backend/utils/cache/inval.c | 581 +++++++++++
src/backend/access/transam/rmgr.c | 3
src/backend/storage/ipc/sinvaladt.c | 10
src/backend/storage/lmgr/lock.c | 44
src/backend/utils/init/flatfiles.c | 55
src/backend/utils/init/postinit.c | 10
src/include/storage/sinval.h | 38
src/include/storage/sinvaladt.h | 2
src/include/utils/flatfiles.h | 9
src/include/utils/inval.h | 4
src/include/access/rmgr.h | 1

Cleanup in recovery
src/backend/access/heap/heapam.c | 88 +
src/backend/commands/vacuum.c | 29
src/backend/commands/vacuumlazy.c | 42

State changes and bgwriter
src/backend/access/transam/xlog.c | 1533 +++++++--!!!!!!!!!!!!!
src/backend/postmaster/bgwriter.c | 418 +-!!!!
src/backend/postmaster/postmaster.c | 97 +
src/backend/storage/buffer/bufmgr.c | 58 +
src/include/miscadmin.h | 6
src/include/postmaster/bgwriter.h | 6
src/include/catalog/pg_control.h | 15
src/include/storage/pmsignal.h | 1

Protections and commands refused
src/backend/commands/discard.c | 3
src/backend/commands/indexcmds.c | 2
src/backend/commands/lockcmds.c | 12
src/backend/commands/sequence.c | 2
src/backend/tcop/postgres.c | 2
src/backend/tcop/utility.c | 20
src/backend/utils/adt/txid.c | 6

New functions (in xlog.c)
src/include/catalog/pg_proc.h | 22

API changes knock-on effects
src/backend/bootstrap/bootstrap.c | 3
src/backend/storage/freespace/freespace.c | 5
src/bin/pg_controldata/pg_controldata.c | 3
src/bin/pg_resetxlog/pg_resetxlog.c | 2

New parameters
src/backend/utils/error/elog.c | 17
src/backend/utils/misc/guc.c | 12

src/test/regress/parallel_schedule | 4
src/test/regress/serial_schedule | 2
62 files changed, 4114 insertions(+), 139 deletions(-), 1586
modifications(!)

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Sat, Nov 1, 2008 at 10:02 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> Hot Standby patch, including all major planned features.
>
>
>
I wonder if we should refactor lazy_scan_heap() so that *all* the real work
of collecting information about dead tuples happens only in
heap_page_prune(). Frankly, there is only a rare chance that a tuple may
become DEAD after the pruning happened on the page. We can ignore such
tuples; they will be vacuumed/pruned in the next cycle.

This would save us a second check of HeapTupleSatisfiesVacuum on the tuples
which are just now checked in heap_page_prune(). In addition, the following
additional WAL records are then not necessary because heap_page_prune() must
have already logged the latestRemovedXid.

+ /*
+ * For Hot Standby we need to know the highest transaction id that will
+ * be removed by any change. VACUUM proceeds in a number of passes so
+ * we need to consider how each pass operates. The first pass runs
+ * heap_page_prune(), which can issue XLOG_HEAP2_CLEAN records as it
+ * progresses - these will have a latestRemovedXid on each record.
+ * In many cases this removes all of the tuples to be removed.
+ * Then we look at tuples to be removed, but do not actually remove them
+ * until phase three. However, index records for those rows are removed
+ * in phase two and index blocks do not have MVCC information attached.
+ * So before we can allow removal of *any* index tuples we need to issue
+ * a WAL record indicating what the latestRemovedXid will be at the end
+ * of phase three. This then allows Hot Standby queries to block at the
+ * correct place, i.e. before phase two, rather than during phase three
+ * as we issue more XLOG_HEAP2_CLEAN records. If we need to run multiple
+ * phase two/three because of memory constraints we need to issue multiple
+ * log records also.
+ */
+ static void
+ vacuum_log_cleanup_info(Relation rel, LVRelStats *vacrelstats)
+ {
+ /*
+ * No need to log changes for temp tables, they do not contain
+ * data visible on the standby server.
+ */
+ if (rel->rd_istemp)
+ return;
+
+ (void) log_heap_cleanup_info(rel->rd_node,
vacrelstats->latestRemovedXid);
+ }

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On Sat, Nov 1, 2008 at 10:02 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> Hot Standby patch, including all major planned features.
>
>
While experimenting with the patch, I noticed that sometimes the archiver
process indefinitely waits for WALInsertLock. I haven't spent much time
debugging that, but the following chunk clearly seems to be buggy. The
WALInsertLock is not released if (leavingArchiveRecovery == true).

--- 6565,6592 ----
}
}

! if (leavingArchiveRecovery)
! checkPoint.redo = GetRedoLocationForArchiveCheckpoint();
! else
{
! /*
! * Compute new REDO record ptr = location of next XLOG record.
! *
! * NB: this is NOT necessarily where the checkpoint record itself
will be,
! * since other backends may insert more XLOG records while we're off
doing
! * the buffer flush work. Those XLOG records are logically after
the
! * checkpoint, even though physically before it. Got that?
! */
! checkPoint.redo = GetRedoLocationForCheckpoint();

! /*
! * Now we can release WAL insert lock, allowing other xacts to
proceed
! * while we are flushing disk buffers.
! */
! LWLockRelease(WALInsertLock);
}

/*
* If enabled, log checkpoint start. We postpone this until now so as
not
* to log anything if we decided to skip the checkpoint.
*/

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On Thu, 2008-11-20 at 11:51 +0530, Pavan Deolasee wrote:

> I wonder if we should refactor lazy_scan_heap() so that *all* the real
> work of collecting information about dead tuples happens only in
> heap_page_prune(). Frankly, there is only a rare chance that a tuple
> may become DEAD after the pruning happened on the page. We can ignore
> such tuples; they will be vacuumed/pruned in the next cycle.
>
> This would save us a second check of HeapTupleSatisfiesVacuum on the
> tuples which are just now checked in heap_page_prune(). In addition,
> the following additional WAL records are then not necessary because
> heap_page_prune() must have already logged the latestRemovedXid.

I like this idea. I've attempted to plug every gap, but perhaps the best
way here is to remove the gap completely.

In my testing, I only saw this case happen a couple of times in many
tests. Rarely executed code gives sporadic bugs, so I would be happy to
remove it and the standby support stuff that goes with it.

I would suggest that we just remove the switch statement:
switch (HeapTupleSatisfiesVacuum(tuple.t_data, OldestXmin, buf))
and alter the following if test since tupgone is also removed.
That will cause HEAPTUPLE_DEAD rows to be fed to heap_freeze_tuple().
Comments on that function claim that is a bad thing, but we know that
any row that was *not* removed by heap_page_prune() and is now dead must
have died very recently and so will never be frozen.

Let me know if you're happy with that change and I'll make it so.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Thu, Nov 20, 2008 at 3:38 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

>
> I would suggest that we just remove the switch statement:
> switch (HeapTupleSatisfiesVacuum(tuple.t_data, OldestXmin, buf))
> and alter the following if test since tupgone is also removed.
> That will cause HEAPTUPLE_DEAD rows to be fed to heap_freeze_tuple().
> Comments on that function claim that is a bad thing, but we know that
> any row that was *not* removed by heap_page_prune() and is now dead must
> have died very recently and so will never be frozen.
>
> Let me know if you're happy with that change and I'll make it so.
>
>

Yeah, I think we should be safe. We continuously hold EX lock on the buffer
since the prune operation is carried out. So the only new DEAD tuples may
arrive because some transaction aborted in between, changing
INSERT_IN_PROGRESS tuple to DEAD. But these tuples won't pass the cutoff_xid
test and should never be frozen.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On Thu, 2008-11-20 at 12:03 +0530, Pavan Deolasee wrote:

> On Sat, Nov 1, 2008 at 10:02 PM, Simon Riggs <simon(at)2ndquadrant(dot)com>
> wrote:
> Hot Standby patch, including all major planned features.
>
>
> While experimenting with the patch, I noticed that sometimes the
> archiver process indefinitely waits for WALInsertLock. I haven't spent
> much time debugging that, but the following chunk clearly seems to be
> buggy. The WALInsertLock is not released if (leavingArchiveRecovery ==
> true).

Mmmm, it seems this is correct. I had to reconstruct this section of
code after recent bitrot, so it looks I introduced a bug doing that.
What I'm surprised about is that I got a similar hang myself in testing
and in my notes I have this ticked as resolved.

The fix is trivial, though I'm sorry it was there for you to find at
all.

(I assume you mean bgwriter, not archiver process).

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Thu, Nov 20, 2008 at 7:50 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

>
> (I assume you mean bgwriter, not archiver process).
>
>
Yeah, its the bgwriter, IIRC hung while taking checkpoint.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

"Pavan Deolasee" <pavan(dot)deolasee(at)gmail(dot)com> writes:
> I wonder if we should refactor lazy_scan_heap() so that *all* the real work
> of collecting information about dead tuples happens only in
> heap_page_prune(). Frankly, there is only a rare chance that a tuple may
> become DEAD after the pruning happened on the page. We can ignore such
> tuples; they will be vacuumed/pruned in the next cycle.

> This would save us a second check of HeapTupleSatisfiesVacuum on the tuples
> which are just now checked in heap_page_prune(). In addition, the following
> additional WAL records are then not necessary because heap_page_prune() must
> have already logged the latestRemovedXid.

I don't think you can do that. Couldn't someone else have run
heap_page_prune between vacuum's first and second visit to the page?

regards, tom lane

On Thu, Nov 20, 2008 at 9:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>
> I don't think you can do that. Couldn't someone else have run
> heap_page_prune between vacuum's first and second visit to the page?
>
>
>

You mean the second visit in the first pass where we again check for
HeapTupleSatisfiesVacuum ? We hold exclusive lock continuously in the first
pass. So its not possible for someone else to call heap_page_prune. If its
the second visit in the second heap scan, then it removes only the dead
tuples recorded in the first pass. So we should be good there too.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On Thu, 2008-11-20 at 10:33 -0500, Tom Lane wrote:
> "Pavan Deolasee" <pavan(dot)deolasee(at)gmail(dot)com> writes:
> > I wonder if we should refactor lazy_scan_heap() so that *all* the real work
> > of collecting information about dead tuples happens only in
> > heap_page_prune(). Frankly, there is only a rare chance that a tuple may
> > become DEAD after the pruning happened on the page. We can ignore such
> > tuples; they will be vacuumed/pruned in the next cycle.
>
> > This would save us a second check of HeapTupleSatisfiesVacuum on the tuples
> > which are just now checked in heap_page_prune(). In addition, the following
> > additional WAL records are then not necessary because heap_page_prune() must
> > have already logged the latestRemovedXid.
>
> I don't think you can do that. Couldn't someone else have run
> heap_page_prune between vacuum's first and second visit to the page?

I just looked at that in more detail and decided it was more difficult
than it first appeared. So I've left it for now.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Thu, Nov 20, 2008 at 8:15 PM, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>wrote:

>
>
> On Thu, Nov 20, 2008 at 7:50 PM, Simon Riggs <simon(at)2ndquadrant(dot)com>wrote:
>
>
>
>>
>> (I assume you mean bgwriter, not archiver process).
>>
>>
> Yeah, its the bgwriter, IIRC hung while taking checkpoint.
>
>
>
Sorry, its the startup process thats stuck in the checkpoint. Here is the
stack trace:

(gdb) bt
#0 0x00110402 in __kernel_vsyscall ()
#1 0x0095564b in semop () from /lib/libc.so.6
#2 0x0825c703 in PGSemaphoreLock (sema=0xb7c52c7c, interruptOK=0 '\0') at
pg_sema.c:420
#3 0x0829ff5e in LWLockAcquire (lockid=WALInsertLock, mode=LW_EXCLUSIVE) at
lwlock.c:456
#4 0x080d5c7e in XLogInsert (rmid=0 '\0', info=16 '\020', rdata=0xbfda1798)
at xlog.c:746
#5 0x080e2e0f in CreateCheckPoint (flags=6) at xlog.c:6674
#6 0x080e1afd in StartupXLOG () at xlog.c:6077
#7 0x08104f2f in AuxiliaryProcessMain (argc=2, argv=0xbfda19e4) at
bootstrap.c:421
#8 0x0826d285 in StartChildProcess (type=StartupProcess) at
postmaster.c:4104
#9 0x082690d9 in PostmasterMain (argc=3, argv=0x9c89a60) at
postmaster.c:1034
#10 0x081f90ff in main (argc=3, argv=0x9c89a60) at main.c:188

ISTM that the postmaster somehow does not receive (may be because of what I
reported in my other mail) PMSIGNAL_RECOVERY_START and hence bgwriter is not
started. The startup process then itself tries to take a checkpoint and
hangs.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On Fri, 2008-11-21 at 17:08 +0530, Pavan Deolasee wrote:

> Sorry, its the startup process thats stuck in the checkpoint. Here is
> the stack trace:

Already fixed in new version I'm preparing for you.

Both the startup process and bgwriter can perform restartpoints, so its
not a problem whether we reached the point at which we pmsignal or not.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Hi,

I found that no one is registered as hot standby reviewer. I'd like
to review the patch, mainly by testing through some benchmark test.

Regards;

2008/11/2 Simon Riggs <simon(at)2ndquadrant(dot)com>:
> Hot Standby patch, including all major planned features.
>
> Allows users to connect to server in archive recovery and run queries.
>
> Applies cleanly, passes make check.
>
> There's no flaky code, kludges or other last minute rush-for-deadline
> stuff. "It all works", though really for a patch this size and scope I
> expect many bugs. As a result I am describing this as "WIP", though it
> is much more than a prototype. All the code has been planned out in
> advance, so there's been very little on-the-fly changes required during
> final development. I'm fully committed to making all required changes
> and fixes in reasonable times. I will continue detailed testing over the
> next few weeks to re-check everything prior to commit.
>
> Initially, I would ask people to spend time thinking about this
> conceptually to check that I have all the correct subsystems and cater
> for all the little side tweaks that exist in various parts of the server
>
> When you test this, please do it on a server built with --enable-cassert
> and keep a detailed log using trace_recovery_messages = DEBUG4 (or 2-3).
>
> Code has been specifically designed to be performance neutral or better
> for normal workloads, so the WAL volume, number of times we take WAL
> locks etc should be identical (on 64-bit systems). The patch includes
> further tuning of subtransaction commits, so I hope the patch may even
> be a few % win on more complex workloads with many PL/pgSQL functions
> using EXCEPTION. Enabling the bgwriter during recovery seems likely to
> be a huge gain on performance for larger shared_buffers settings, which
> should offset a considerable increase in CPU usage during recovery.
>
> Performance test results would be appreciated
> * for normal running - just to test that it really is neutral
> * for recovery - if query access not used is it faster than before
>
> Points of significant note for detailed reviewers
>
> * Prepared transactions not implemented yet. No problems foreseen, but
> want to wait to see if other refactorings are required.
>
> * Touching every block of a btree index during a replay of VACUUM is not
> yet implemented. Would like some second opinions that it is even
> required. I have code prototyped for it, but it feels too wacky even
> after Heikki and I agreed it was required. Do we still think that?
>
> * locking correctness around flat file refresh still not enabled
>
> * need some discussiona round how to handle max_connections changes
> cleanly.

--
------
Koichi Suzuki

On Wed, Dec 3, 2008 at 11:00 AM, Koichi Suzuki <koichi(dot)szk(at)gmail(dot)com> wrote:

> Hi,
>
> I found that no one is registered as hot standby reviewer. I'd like
> to review the patch, mainly by testing through some benchmark test.
>
>
You can yourself edit the Wiki page, though you need to register first. But
its very straight forward.

I added myself as reviewer to Hot standby few days back since I did some
code review and testing. I intend to spend some more time after new patch is
posted.

Thanks,
Pavan

Pavan Deolasee.
EnterpriseDB http://www.enterprisedb.com

Thank you for your advise. I'll edit the Wiki page.

2008/12/3 Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>:
>
>
> On Wed, Dec 3, 2008 at 11:00 AM, Koichi Suzuki <koichi(dot)szk(at)gmail(dot)com> wrote:
>>
>> Hi,
>>
>> I found that no one is registered as hot standby reviewer. I'd like
>> to review the patch, mainly by testing through some benchmark test.
>>
>
> You can yourself edit the Wiki page, though you need to register first. But
> its very straight forward.
>
> I added myself as reviewer to Hot standby few days back since I did some
> code review and testing. I intend to spend some more time after new patch is
> posted.
>
> Thanks,
> Pavan
>
> Pavan Deolasee.
> EnterpriseDB http://www.enterprisedb.com
>

--
------
Koichi Suzuki