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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
