Pause/Resume feature for Hot Standby
in [PgSql]
|
From: Robert Haas on 4 May 2010 11:33 On Tue, May 4, 2010 at 11:10 AM, Simon Riggs <simon(a)2ndquadrant.com> wrote: > This is clearly a response to issues raised about HS and not a new > feature. I don't find that clear at all. In fact, I find the exact opposition position to be clear. > It's also proposed in the most minimal way possible with > respect for the current state of release. Why is you think I want to go > to beta less quickly than anyone else? We're already in beta. I said nothing about when you want to go to beta or do anything else. > There hasn't been anything more than a minor bug in weeks, so not really > sure how you arrive at that the idea the code needs "stabilising". I don't agree that there hasn't been anything more than a minor bug in weeks. I arrive at the idea that the code needs stabilizing on the basis of the fact that we keep finding new bugs. > When people complain, I propose solutions. If you then object that the > proposed solution is actually a new feature, that leaves us in a > deadlock. Not really. You're entitled to say what you think we should do and I am entitled to say what I think we should do. I think we should wait for 9.1. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 4 May 2010 13:23 Simon Riggs <simon(a)2ndQuadrant.com> writes: > There hasn't been anything more than a minor bug in weeks, so not really > sure how you arrive at that the idea the code needs "stabilising". Simon, if you don't think the code needs stabilizing, you need to think again. * max_standby_delay logic is broken, as per other thread. * handle_standby_sig_alarm is broken to the point of needing to be thrown away; you can NOT do that kind of thing in an interrupt handler. * RecordKnownAssignedTransactionIds is changing ShmemVariableCache->nextXid without any kind of lock (in general, I suspect all the xlog replay code needs to be revisited to see if it's skipping locks on shared data structures that are now potentially going to be examined by backends) * Use of StandbyTransactionIdIsPrepared seems awfully dubious: why are we trusting the standby's pg_twophase files more than data from the WAL log, *especially* before we have reached consistency? Not to mention that that's a horridly expensive operation (filesystem access) being invoked while holding ProcArrayLock. * Why is ExtendCLOG/ExtendSUBTRANS done in RecordKnownAssignedTransactionIds? It's inappropriate from a modularity standpoint, and it also seems completely wrong that it won't get done if standbyState < STANDBY_SNAPSHOT_PENDING. nextXID manipulation there seems equally bogus not to mention unlocked. * snapshotOldestActiveXid is bogus (I complained about this already, you have not fixed it) * LogStandbySnapshot is merest fantasy: no guarantee that either the XIDs list or the locks list will be consistent with the point in WAL where it will get inserted. What's worse, locking things down enough to guarantee consistency would be horrid for performance, or maybe even deadlock-inducing. Could lose both ways: list might contain an XID whose commit/abort went to WAL before the snapshot did, or list might be missing an XID started just after snap was taken, The latter case could possibly be dealt with via nextXid filtering, but that doesn't fix the former case, and anyway we have both ends of the same problem for locks. That's just what I found in a day or so of code reading, and I haven't read anything like all of the HS patches. You need to stop thinking about adding features and start thinking about making what's in there bulletproof. If you happen to have an idle moment when you're not fixing known problems, re-read some code. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Simon Riggs on 4 May 2010 13:46 On Tue, 2010-05-04 at 13:23 -0400, Tom Lane wrote: > Simon Riggs <simon(a)2ndQuadrant.com> writes: > > There hasn't been anything more than a minor bug in weeks, so not really > > sure how you arrive at that the idea the code needs "stabilising". > > Simon, if you don't think the code needs stabilizing, you need to think > again. This list is entirely new to me. I can't fix problems you haven't even raised before, can I? Why have you been saving that list?? No way are these "known problems". > That's just what I found in a day or so of code reading, and I haven't > read anything like all of the HS patches. You need to stop thinking > about adding features and start thinking about making what's in there > bulletproof. If you happen to have an idle moment when you're not > fixing known problems, re-read some code. Nobody is adding new features. Stop barracking me for something that's not even happening, especially if you persuade yourself you should be angry about it. I care as much about beta as anyone else. Yes, I'll go read your list. Thank you for your review. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Simon Riggs on 4 May 2010 16:06 On Tue, 2010-05-04 at 13:23 -0400, Tom Lane wrote: > * max_standby_delay logic is broken, as per other thread. Proposed fix submitted, > * handle_standby_sig_alarm is broken to the point of needing to be > thrown away; you can NOT do that kind of thing in an interrupt handler. This was modelled very closely on handle_sig_alarm() and was reviewed by other hackers. I'm not great on that, as you know, so if you can explain what it is I can't do, and how that differs from handle_sig_alarm running the deadlock detector in the same way, then I'll work on it some more. > * RecordKnownAssignedTransactionIds is changing ShmemVariableCache->nextXid > without any kind of lock (in general, I suspect all the xlog replay code > needs to be revisited to see if it's skipping locks on shared data > structures that are now potentially going to be examined by backends) There is only one writer and this a single integer value, so any reads are atomic. This is not being used as a memory barrier, so our earlier discussion about weak-memory ordering doesn't apply. The only other reader is bgwriter. I'm happy to add additional locking if you think its really needed. > * Use of StandbyTransactionIdIsPrepared seems awfully dubious: why are > we trusting the standby's pg_twophase files more than data from the WAL > log, *especially* before we have reached consistency? StandbyTransactionIdIsPrepared() is only called in two places, both of which relate to pruning the KnownAssignedXids array. Pruning only occurs when the WAL log specifically does not contain the information we need, which only occurs when those hypothetical FATAL errors come along. In that case we rely upon the pg_twophase files. Both of those call points happen in ProcArrayApplyRecoveryInfo() which does get called before we are consistent, though we can change that if you see a problem. At this point, I don't see an issue. > Not to mention > that that's a horridly expensive operation (filesystem access) being > invoked while holding ProcArrayLock. I just optimised that in the recent patch you committed. It isn't a high cost item any longer now that we are able to prune KnownAssignedXids() from the left, since pruning will typically not test more than one xid. > * Why is ExtendCLOG/ExtendSUBTRANS done in RecordKnownAssignedTransactionIds? Heikki placed them there, so I left that coding, since it does work. RecordKnown..() is supposed to be the logical equivalent of assigning an xid, so it seemed logical. Happy to move wherever you see fit. > It's inappropriate from a modularity standpoint, and it also seems completely > wrong that it won't get done if standbyState < STANDBY_SNAPSHOT_PENDING. Yes, that looks like a logic error and will be fixed. However, its trapped later by clog code to zero new blocks, so in practice there is no bug. > nextXID manipulation there seems equally bogus not to mention unlocked. Traced the code, looks fine to me. Yes, unlocked. > * snapshotOldestActiveXid is bogus (I complained about this > already, you have not fixed it) I understood you were fixing it, as raised during your recent review of the KAX patch. Will fix. > * LogStandbySnapshot is merest fantasy: no guarantee that either the XIDs > list or the locks list will be consistent with the point in WAL where it > will get inserted. What's worse, locking things down enough to guarantee > consistency would be horrid for performance, or maybe even deadlock-inducing. > Could lose both ways: list might contain an XID whose commit/abort went > to WAL before the snapshot did, or list might be missing an XID started > just after snap was taken, The latter case could possibly be dealt with > via nextXid filtering, but that doesn't fix the former case, and anyway > we have both ends of the same problem for locks. That was recoded by Heikki and I left it as written, though I checked it, considered it correct and take responsibility for it. Will review further and report back. Thanks for the review. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
First
|
Prev
|
Pages: 1 2 Prev: [HACKERS] Pause/Resume feature for Hot Standby Next: Reg: SQL Query for Postgres 8.4.3 |