Thread: 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
Attachment
<br /><br /><div class="gmail_quote">On Sat, Nov 1, 2008 at 10:02 PM, Simon Riggs <span dir="ltr"><<a
href="mailto:simon@2ndquadrant.com">simon@2ndquadrant.com</a>></span>wrote:<br /><blockquote class="gmail_quote"
style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> Hot Standby patch,
includingall major planned features.<br /><br /><br /></blockquote></div><br />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.<br /><br />This would save us a second
checkof HeapTupleSatisfiesVacuum on the tuples which are just now checked in heap_page_prune(). In addition, the
followingadditional WAL records are then not necessary because heap_page_prune() must have already logged the
latestRemovedXid.<br/><br />+ /*<br />+ * For Hot Standby we need to know the highest transaction id that will<br />+
*be removed by any change. VACUUM proceeds in a number of passes so<br />+ * we need to consider how each pass
operates.The first pass runs<br /> + * heap_page_prune(), which can issue XLOG_HEAP2_CLEAN records as it<br />+ *
progresses- these will have a latestRemovedXid on each record.<br />+ * In many cases this removes all of the tuples
tobe removed.<br />+ * Then we look at tuples to be removed, but do not actually remove them<br /> + * until phase
three.However, index records for those rows are removed<br />+ * in phase two and index blocks do not have MVCC
informationattached.<br />+ * So before we can allow removal of *any* index tuples we need to issue<br /> + * a WAL
recordindicating what the latestRemovedXid will be at the end<br />+ * of phase three. This then allows Hot Standby
queriesto block at the<br />+ * correct place, i.e. before phase two, rather than during phase three<br /> + * as we
issuemore XLOG_HEAP2_CLEAN records. If we need to run multiple<br />+ * phase two/three because of memory constraints
weneed to issue multiple<br />+ * log records also.<br />+ */<br />+ static void<br />+
vacuum_log_cleanup_info(Relationrel, LVRelStats *vacrelstats)<br /> + {<br />+ /*<br />+ * No need to log changes
fortemp tables, they do not contain<br />+ * data visible on the standby server.<br />+ */<br />+ if
(rel->rd_istemp)<br/>+ return;<br />+<br />+ (void) log_heap_cleanup_info(rel->rd_node,
vacrelstats->latestRemovedXid);<br/> + }<br /><br /><br />Thanks,<br />Pavan<br clear="all" /><br />-- <br />Pavan
Deolasee<br/>EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br />
<br /><br /><div class="gmail_quote">On Sat, Nov 1, 2008 at 10:02 PM, Simon Riggs <span dir="ltr"><<a
href="mailto:simon@2ndquadrant.com">simon@2ndquadrant.com</a>></span>wrote:<br /><blockquote class="gmail_quote"
style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> Hot Standby patch,
includingall major planned features.<br /><br /></blockquote></div><br />While experimenting with the patch, I noticed
thatsometimes the archiver process indefinitely waits for WALInsertLock. I haven't spent much time debugging that, but
thefollowing chunk clearly seems to be buggy. The WALInsertLock is not released if (leavingArchiveRecovery == true).<br
/><br/>--- 6565,6592 ----<br /> }<br /> }<br /><br />! if (leavingArchiveRecovery)<br />!
checkPoint.redo= GetRedoLocationForArchiveCheckpoint();<br />! else<br /> {<br />! /*<br />! *
Computenew REDO record ptr = location of next XLOG record.<br /> ! *<br />! * NB: this is NOT necessarily
wherethe checkpoint record itself will be,<br />! * since other backends may insert more XLOG records while
we'reoff doing<br />! * the buffer flush work. Those XLOG records are logically after the<br /> ! *
checkpoint,even though physically before it. Got that?<br />! */<br />! checkPoint.redo =
GetRedoLocationForCheckpoint();<br/><br />! /*<br />! * Now we can release WAL insert lock, allowing other
xactsto proceed<br /> ! * while we are flushing disk buffers.<br />! */<br />!
LWLockRelease(WALInsertLock);<br/> }<br /><br /> /*<br /> * If enabled, log checkpoint start. We postpone
thisuntil now so as not<br /> * to log anything if we decided to skip the checkpoint.<br /> */<br /><br /><br
/>Thanks,<br/>Pavan<br clear="all" /><br />-- <br />Pavan Deolasee<br />EnterpriseDB <a
href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/>
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.comPostgreSQL Training, Services and Support
On Thu, Nov 20, 2008 at 3:38 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
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
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@2ndquadrant.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.comPostgreSQL Training, Services and Support
On Thu, Nov 20, 2008 at 7:50 PM, Simon Riggs <simon@2ndquadrant.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.deolasee@gmail.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, 2008-11-20 at 10:33 -0500, Tom Lane wrote: > "Pavan Deolasee" <pavan.deolasee@gmail.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.comPostgreSQL Training, Services and Support
<br /><br /><div class="gmail_quote">On Thu, Nov 20, 2008 at 9:03 PM, Tom Lane <span dir="ltr"><<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>></span>wrote:<br /><blockquote class="gmail_quote" style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d"><br /></div>Idon't think you can do that. Couldn't someone else have run<br /> heap_page_prune between vacuum's first and secondvisit to the page?<br /><br /> <br /></blockquote></div><br />You mean the second visit in the first pass where weagain check for HeapTupleSatisfiesVacuum ? We hold exclusive lock continuously in the first pass. So its not possible forsomeone else to call heap_page_prune. If its the second visit in the second heap scan, then it removes only the deadtuples recorded in the first pass. So we should be good there too.<br /><br />Thanks,<br />Pavan<br clear="all" /><br/>-- <br />Pavan Deolasee<br />EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/>
On Thu, Nov 20, 2008 at 8:15 PM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:
Yeah, its the bgwriter, IIRC hung while taking checkpoint.On Thu, Nov 20, 2008 at 7:50 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
(I assume you mean bgwriter, not archiver process).
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.comPostgreSQL 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@2ndquadrant.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
<br /><br /><div class="gmail_quote">On Wed, Dec 3, 2008 at 11:00 AM, Koichi Suzuki <span dir="ltr"><<a href="mailto:koichi.szk@gmail.com">koichi.szk@gmail.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> Hi,<br /><br /> I foundthat no one is registered as hot standby reviewer. I'd like<br /> to review the patch, mainly by testing through somebenchmark test.<br /><br /></blockquote></div><br />You can yourself edit the Wiki page, though you need to registerfirst. But its very straight forward.<br /><br />I added myself as reviewer to Hot standby few days back since Idid some code review and testing. I intend to spend some more time after new patch is posted. <br /><br />Thanks,<br />Pavan<br/><br />Pavan Deolasee.<br />EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/>
Thank you for your advise. I'll edit the Wiki page. 2008/12/3 Pavan Deolasee <pavan.deolasee@gmail.com>: > > > On Wed, Dec 3, 2008 at 11:00 AM, Koichi Suzuki <koichi.szk@gmail.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