Hot Standby 0.2.1
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.
OVERVIEW
You can download PDF versions of the fine manual is here
http://wiki.postgresql.org/images/0/01/Hot_Standby_main.pdf
http://wiki.postgresql.org/images/0/01/Hot_Standby_Recovery_Functions.pdf
Also available via the project Wiki, which is here
http://wiki.postgresql.org/wiki/Hot_Standby
Patch should be attached to this email. If problems, get this and other
versions from Wiki please. No offlist comments, questions etc please.
PATCH VERSIONING & STATUS
0 meaning its not fully released yet
2 meaning this is a major new re-write
1 meaning this is the first release
Patch is still in testing and will be for next few days at least.
Released now only because I promised to do so.
Is this ready for commit? Yes, it is in the shape I want it to be in,
but also, No, I can't say it's been through a wide enough range of tests
as yet to be considered immediately ready for commit.
Further bug fixing and minor cosmetic development will take place via my
GIT repo, uploaded soon. Patch included here to meet deadlines and code
inclusion. BSD.
PATCH SUMMARY
* 76 files changed, 5160 insertions(+), 59 deletions(-), 1251 mods(!)
* 7 files with more than 100 lines changed
procarray.c (1200+ additions)
xlog.c (600+ additions)
xact.c (500+ additions)
inval.c (650 additions)
lock.c
heapam.c
nbtxlog.c
* 29 files with 10 or fewer lines changed
* Applies cleanly to CVS HEAD as of now
CHANGES
The rough changes since version 1 series of patches.
* Full documentation included. Many, but not all nuances of SGML tagging
followed, but sufficient aspects there to allow for proofreading before
we do final changes. Some undocumented functions now correctly
documented. Recovery functions now split into user and control functions
in docs to make it clearer.
* Starting conditions in GetRunningTransactionData() are now much
stricter and holds more lwlocks. There are few cases where any not-found
xids are allowed during xid processing, so code is more robust. Please
check for race conditions.
* GetRunningTransactionData() now handles initialisation of
AccessExclusiveLocks correctly. Locks are counted in a low-contention
approach that avoids taking holding lock partition locks, if possible.
* Start-up conditions now recoded to allow faster start in cases where
many subtransactions are present. Recovery connections are only enabled
when the snapshot is valid.
* max_connections needs to be correctly set or HS will not allow
connections. Once snapshots are enabled they will continue to be
available always.
* RecordKnownAssignedTransactions() now contains a test for xid
wraparound threat which invokes conflict processing should that occur.
* Boolean states now clarified and corrected. Hot Standby can be turned
off completely if not required or if problems effect production. That
causes many changes but there is no change in the intention of those
sections of code.
* UnobservedXids processing follows Heikki's proposal, but has been
renamed to KnownAssignedXids. It has also been modularised and
completely re-written using a hash table approach. So far it has been
much more stable than the previous sorted array coding, which I am happy
to see in the shredder for all the problems it caused. Fully detailed
comments all through.
* All record types now respect max_standby_delay.
* All deferred conflict processing has been removed - conflict
processing itself is still enabled.
* A few other functions have been renamed and/or moved around to
rationalise their exact purpose/position within their modules.
* Prepared transactions holding AccessExclusiveLocks at the end of
recovery are now handled.
* Hash indexes are now safely handled. That was removed at request, but
we need it to avoid silent data loss for queries near those types of
index.
* Hint bits are now set, in appropriate circumstances.
* Flat file logic removed
* Large swathes of unused code removed.
* All code comments addressed and/or re-explained in more detail
* CHECKPOINT now works during recovery but performs restartpoint
instead.
* Tweaked max_standby_delay code to avoid long duration waits. Added
dynamic function to control delay during recovery. Added code for stats
collection and ps display. Set default to sensible production values.
RECENT BUGS
* Found and fixed missing relcache init file invalidation
* Found and fixed more serious VACUUM FULL-related weirdness <sigh>
* Recently discovered bug has resulted in changes in
XidInMVCCSnapshot(). Heikki's earlier approach did not correctly allow
for the maximum size of a snapshot. The simplicity of Heikki's proposal
is good, but hid a flaw in where snapshots would put their xids. I've
fully solved the problem though I expect further discussion.
I've looked through every change and verified it, but fixing all the
bugs means there's areas of new code added in last few days. I accept
that any bugs herein are my responsibility.
--
Simon Riggs www.2ndQuadrant.com
Attachments:
hot_standby.riggs.v0.2.1.patch.bz2application/x-bzip; name=hot_standby.riggs.v0.2.1.patch.bz2Download+1-0
On Tue, Sep 15, 2009 at 10:41:59PM +0100, 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.
Kudos!!!!
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--On 15. September 2009 22:41:59 +0100 Simon Riggs <simon@2ndQuadrant.com>
wrote:
http://wiki.postgresql.org/images/0/01/Hot_Standby_Recovery_Functions.pdf
This doesn't work for me, it seems the correct link is
<http://wiki.postgresql.org/images/1/10/Hot_Standby_Recovery_Functions.pdf>
?
--
Thanks
Bernd
All,
Now that Simon has submitted this, can some of the heavy-hitters here
review it? Heikki?
Nobody's name is next to it.
--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com
On Wed, Sep 16, 2009 at 6:05 PM, Josh Berkus <josh@agliodbs.com> wrote:
Now that Simon has submitted this, can some of the heavy-hitters here
review it? Heikki?Nobody's name is next to it.
I don't think anyone is planning to ignore this patch, but it wasn't
included in the first round of round-robin reviewing assignments
because it wasn't submitted until the following day, after the
announced deadline for submissions had already passed. So most people
are probably busy with with some other patch at the moment, but that's
a temporary phenomenon. This is a pretty small CommitFest, so there
shouldn't be any shortage of reviewers, though Heikki's time may be
stretched a little thin, since Streaming Replication is also in the
queue, and he is working on index-only scans. That's really for him
to comment on, though.
...Robert
Robert Haas wrote:
On Wed, Sep 16, 2009 at 6:05 PM, Josh Berkus <josh@agliodbs.com> wrote:
Now that Simon has submitted this, can some of the heavy-hitters here
review it? Heikki?Nobody's name is next to it.
I don't think anyone is planning to ignore this patch, but it wasn't
included in the first round of round-robin reviewing assignments
because it wasn't submitted until the following day, after the
announced deadline for submissions had already passed. So most people
are probably busy with with some other patch at the moment, but that's
a temporary phenomenon.
Right, I've added myself as reviewer now.
This is a pretty small CommitFest, so there
shouldn't be any shortage of reviewers, though Heikki's time may be
stretched a little thin, since Streaming Replication is also in the
queue, and he is working on index-only scans. That's really for him
to comment on, though.
I'm going to put the index-only scans aside for now to focus on hot
standby and streaming replication. Both are big patches, so there's
plenty of work in those two alone, and not only for me.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Thu, 2009-09-17 at 09:54 +0300, Heikki Linnakangas wrote:
This is a pretty small CommitFest, so there
shouldn't be any shortage of reviewers, though Heikki's time may be
stretched a little thin, since Streaming Replication is also in the
queue, and he is working on index-only scans. That's really for him
to comment on, though.I'm going to put the index-only scans aside for now to focus on hot
standby and streaming replication. Both are big patches, so there's
plenty of work in those two alone, and not only for me.
That's very good of you, thanks.
It was already clear to a few people that your time would bottleneck
trying to review both at the same time. I personally wasn't expecting
you to jump into immediate action on HS. We have time.
--
Simon Riggs www.2ndQuadrant.com
On Thu, Sep 17, 2009 at 2:54 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
Robert Haas wrote:
On Wed, Sep 16, 2009 at 6:05 PM, Josh Berkus <josh@agliodbs.com> wrote:
Now that Simon has submitted this, can some of the heavy-hitters here
review it? Heikki?Nobody's name is next to it.
I don't think anyone is planning to ignore this patch, but it wasn't
included in the first round of round-robin reviewing assignments
because it wasn't submitted until the following day, after the
announced deadline for submissions had already passed. So most people
are probably busy with with some other patch at the moment, but that's
a temporary phenomenon.Right, I've added myself as reviewer now.
This is a pretty small CommitFest, so there
shouldn't be any shortage of reviewers, though Heikki's time may be
stretched a little thin, since Streaming Replication is also in the
queue, and he is working on index-only scans. That's really for him
to comment on, though.I'm going to put the index-only scans aside for now to focus on hot
standby and streaming replication. Both are big patches, so there's
plenty of work in those two alone, and not only for me.
What is the best way to attack this? Should I keep reviewing
index-only scans so that you have feedback for when you get back to
it, or should I move on to something else? If something else, does it
make more sense for me to look at HS since I did a bit of work with a
previous version, or would it be better for me to just pick one of the
other patches from the CommitFest and work on that?
Also, stepping back from me personally, should we try to assign some
additional reviewers to these patches? Is there some way we can
divide up review tasks among multiple people so that we're not
repeating each others work?
Thoughts appreciated, from Heikki, Simon, or others.
...Robert
On Thu, 2009-09-17 at 19:01 -0400, Robert Haas wrote:
I'm going to put the index-only scans aside for now to focus on hot
standby and streaming replication. Both are big patches, so there's
plenty of work in those two alone, and not only for me.What is the best way to attack this? Should I keep reviewing
index-only scans so that you have feedback for when you get back to
it, or should I move on to something else? If something else, does it
make more sense for me to look at HS since I did a bit of work with a
previous version, or would it be better for me to just pick one of the
other patches from the CommitFest and work on that?Also, stepping back from me personally, should we try to assign some
additional reviewers to these patches? Is there some way we can
divide up review tasks among multiple people so that we're not
repeating each others work?Thoughts appreciated, from Heikki, Simon, or others.
I think this is a great opportunity to widen the pool of people
contributing to reviews.
I suggest the creation of a second group of people, performing
round-robin testing of patches. These people would be able to verify
* documentation matches implemented features (does it do what it says on
the tin?)
* usability of explicit features (do the features work well?)
* usability gap of unimplemented features (what else do we need?)
* are there any bugs?
These questions are often quickly answered for smaller patches, but HS's
scope mean that such a task properly executed could take a full week, if
not longer.
Second group of people are just as skilled Postgres people as reviewers,
in some cases more so, apart from they have less detailed knowledge of
the codebase. We have many such people and it would be good to encourage
them to perform thorough reviews rather than "tire kicking".
I'm not sure that Heikki needs additional reviewers. He now has
significant knowledge of the patch and is good at focusing on key
aspects of the internals. Other code reviewers are welcome, of course.
--
Simon Riggs www.2ndQuadrant.com
Robert Haas <robertmhaas@gmail.com> writes:
Also, stepping back from me personally, should we try to assign some
additional reviewers to these patches? Is there some way we can
divide up review tasks among multiple people so that we're not
repeating each others work?Thoughts appreciated, from Heikki, Simon, or others.
How about this proposal:
http://archives.postgresql.org/pgsql-hackers/2009-08/msg00764.php
Regards,
--
dim
On Tue, Sep 15, 2009 at 2:41 PM, Simon Riggs <simon@2ndquadrant.com> 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.
Hi Simon,
Is there a reason that you remove the WAL_DEBUG shown below?
*************** begin:;
*** 899,923 ****
FIN_CRC32(rdata_crc);
record->xl_crc = rdata_crc;
- #ifdef WAL_DEBUG
- if (XLOG_DEBUG)
- {
- StringInfoData buf;
-
- initStringInfo(&buf);
- appendStringInfo(&buf, "INSERT @ %X/%X: ",
- RecPtr.xlogid,
RecPtr.xrecoff);
- xlog_outrec(&buf, record);
- if (rdata->data != NULL)
- {
- appendStringInfo(&buf, " - ");
- RmgrTable[record->xl_rmid].rm_desc(&buf,
record->xl_info, rdata->data);
- }
- elog(LOG, "%s", buf.data);
- pfree(buf.data);
- }
- #endif
-
/* Record begin of record in appropriate places */
ProcLastRecPtr = RecPtr;
Insert->PrevRecord = RecPtr;
--- 947,952 ----Thanks,
Jeff
On Fri, 2009-09-18 at 07:23 -0700, Jeff Janes wrote:
On Tue, Sep 15, 2009 at 2:41 PM, Simon Riggs <simon@2ndquadrant.com>
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.
Is there a reason that you remove the WAL_DEBUG shown below?
WAL_DEBUG is not removed by the patch, though that section of code is
removed, as you observe. I recall an earlier bug report by
me/conversation on hackers about how that section of code was
irrecoverably broken, since it's calling an rmgr routine while not in
recovery and also assuming the data is fully accessible at that point,
which it is not.
--
Simon Riggs www.2ndQuadrant.com
Simon Riggs <simon@2ndQuadrant.com> writes:
On Fri, 2009-09-18 at 07:23 -0700, Jeff Janes wrote:
Is there a reason that you remove the WAL_DEBUG shown below?
WAL_DEBUG is not removed by the patch, though that section of code is
removed, as you observe. I recall an earlier bug report by
me/conversation on hackers about how that section of code was
irrecoverably broken, since it's calling an rmgr routine while not in
recovery and also assuming the data is fully accessible at that point,
which it is not.
Wouldn't it be sufficient to remove the rm_desc() call? I agree
that that's broken, but the rest doesn't seem to be.
regards, tom lane
On Fri, 2009-09-18 at 11:14 -0400, Tom Lane wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
On Fri, 2009-09-18 at 07:23 -0700, Jeff Janes wrote:
Is there a reason that you remove the WAL_DEBUG shown below?
WAL_DEBUG is not removed by the patch, though that section of code is
removed, as you observe. I recall an earlier bug report by
me/conversation on hackers about how that section of code was
irrecoverably broken, since it's calling an rmgr routine while not in
recovery and also assuming the data is fully accessible at that point,
which it is not.Wouldn't it be sufficient to remove the rm_desc() call? I agree
that that's broken, but the rest doesn't seem to be.
That would make sense also. Previous action just because that was
earlier consensus. Will change.
--
Simon Riggs www.2ndQuadrant.com
I want to help on this area, but I need a mentor for this.
For example, Heikki will be a excellent mentor for me.
Following the theme, I think that we have to wide all questions for the process of the acceptance of a patch on the same way that you Simon.
We could write new requirements with all these ideas. Don´t you think?
Regards
"The hurry is enemy of the success: for that reason.......Be patient"
Ing. Marcos L. Ortiz Valmaseda
Línea Soporte y Despliegue
Centro de Tecnologías de Almacenamiento y Análisis de Datos (CENTALAD)
Linux User # 418229
PostgreSQL User
http://www.postgresql.org
http://www.planetpostgresql.org/
http://www.postgresql-es.org/
----- Mensaje original -----
De: "Simon Riggs" <simon@2ndQuadrant.com>
Para: "Robert Haas" <robertmhaas@gmail.com>
CC: "Heikki Linnakangas" <heikki.linnakangas@enterprisedb.com>, "Josh Berkus" <josh@agliodbs.com>, pgsql-hackers@postgresql.org
Enviados: Jueves, 17 de Septiembre 2009 20:53:24 GMT -10:00 Hawai
Asunto: Re: [HACKERS] Hot Standby 0.2.1
On Thu, 2009-09-17 at 19:01 -0400, Robert Haas wrote:
I'm going to put the index-only scans aside for now to focus on hot
standby and streaming replication. Both are big patches, so there's
plenty of work in those two alone, and not only for me.What is the best way to attack this? Should I keep reviewing
index-only scans so that you have feedback for when you get back to
it, or should I move on to something else? If something else, does it
make more sense for me to look at HS since I did a bit of work with a
previous version, or would it be better for me to just pick one of the
other patches from the CommitFest and work on that?Also, stepping back from me personally, should we try to assign some
additional reviewers to these patches? Is there some way we can
divide up review tasks among multiple people so that we're not
repeating each others work?Thoughts appreciated, from Heikki, Simon, or others.
I think this is a great opportunity to widen the pool of people
contributing to reviews.
I suggest the creation of a second group of people, performing
round-robin testing of patches. These people would be able to verify
* documentation matches implemented features (does it do what it says on
the tin?)
* usability of explicit features (do the features work well?)
* usability gap of unimplemented features (what else do we need?)
* are there any bugs?
These questions are often quickly answered for smaller patches, but HS's
scope mean that such a task properly executed could take a full week, if
not longer.
Second group of people are just as skilled Postgres people as reviewers,
in some cases more so, apart from they have less detailed knowledge of
the codebase. We have many such people and it would be good to encourage
them to perform thorough reviews rather than "tire kicking".
I'm not sure that Heikki needs additional reviewers. He now has
significant knowledge of the patch and is good at focusing on key
aspects of the internals. Other code reviewers are welcome, of course.
--
Simon Riggs www.2ndQuadrant.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
hs-minor-fixes-1.patchtext/x-diff; name=hs-minor-fixes-1.patchDownload+47-94
On Mon, 2009-09-21 at 13:50 +0300, Heikki Linnakangas wrote:
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.
OK, I agree with that. Straightforward change. Thanks very much.
I marked the comment to indicate that the handling for GIST and GIN
indexes looked dubious to me also. I had the earlier "it is safe"
comments but that was before we looked at the kill prior tuples issue.
Re-reading code for GIN also, I note that there isn't any further work
because we don't kill prior tuples ever. Also, there is no special
handling of the GIN b-tree posting tree because VACUUM scans that in
logical sequence, rather than the physical sequence in nbtree.
--
Simon Riggs www.2ndQuadrant.com
On Mon, 2009-09-21 at 13:50 +0300, Heikki Linnakangas wrote:
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_onlyseems 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.
Yes, clearly missing a check there. Those two operations should be
blocked at higher level, using PreventCommandDuringRecovery() and I
confess that I thought they already were. Doesn't crash because of the
other checks in place, but gives wrong error message.
Thanks for penetration testing the patch.
--
Simon Riggs www.2ndQuadrant.com
On Mon, 2009-09-21 at 14:01 +0100, Simon Riggs wrote:
On Mon, 2009-09-21 at 13:50 +0300, Heikki Linnakangas wrote:
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.OK, I agree with that. Straightforward change. Thanks very much.
I marked the comment to indicate that the handling for GIST and GIN
indexes looked dubious to me also. I had the earlier "it is safe"
comments but that was before we looked at the kill prior tuples issue.
ISTM I looked at this too quickly.
kill_prior_tuple is only ever set by these lines, after scan starts:
if (!scan->xactStartedInRecovery)
scan->kill_prior_tuple = scan->xs_hot_dead;
which is set in indexam.c, not within any particular am. So the coding,
as submitted, covers all index types, current and future.
AFAICS there is no bug, unless you have a test case or can explain
further?
Worth raising as a query because it forced me to re-check how GIST and
GIN work and am happy again now.
--
Simon Riggs www.2ndQuadrant.com
