Re: Pause/Resume feature for Hot Standby

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