Re: Hot Standby 0.2.1

On Fri, 2009-09-25 at 11:49 +0300, Heikki Linnakangas wrote:
> Looking at the startup sequence now.
>
> I see that you modified ExtendSUBTRANS so that it doesn't wipe out
> previously set values if it's called with out-of-order xids. I guess
> that works, although I think it can leave pages unzeroed if it's called
> with a large enough gap between xids, so that the previous one was on
> e.g page 10 and the next one on page 12. Page 11 would not be zeroed,
> AFAICS. Not sure if such big gaps in the xid sequence are possible, but
> seems so if you have a very large max_connections setting and a lot of
> subtransactions.

Yeh, it happens and I've seen it - which is why the code is there.

> Nevertheless, I'd feel better if we kept ExtendSUBTRANS unmodified, and
> instead arranged things so that ExtendSUBTRANS is always called in
> xid-order. We can call it from RecordKnownAssignedTransactionIds, which
> handles the out-of-order problem for other purposes already.
>
> I think we have similar problem with clog. ExtendCLOG is currently not
> called during recovery, so isn't it possible for the problem with
> subtransaction commits that's described in the comments in StartupCLOG
> to arise during hot standby? Again, I think we should call ExtendCLOG()
> in RecordKnownAssignedTransactionIds. RecordKnownAssignedTransactionIds
> is the hot standby version of GetNewTransactionId(), so to speak.

OK. We record xids in sequence, so it is now a much more natural place
to do this. Zeroing them makes them dirty, unfortunately, but that is
another issue.

RecordKnownAssignedTransactionIds() only works once the snapshot has
been initialised. That could leave a gap, so we will need to run it
continuously if InHotStandby. Which may have knock-on changes also.

> If we go with that, I think we'll need to call StartupSUBTRANS/CLOG
> earlier in the startup sequence too, when we establish the startup
> snapshot and let backends in.

Yes, I think an earlier patch had that, but it turns out that there
really isn't anything for those functions to do. Really we could rename
those functions EndOfRecoverySUBTRANS and EndOfRecoveryCLOG to
illustrate their role better.

> - I removed the new DBConsistentUpToLSN() function and moved its
> functionality into XLogNeedsFlush(). Since XLogFlush() updates
> minRecoveryPoint instead of flushing WAL during recovery, it makes sense
> for XLogNeedsFlush() to correspondingly check if minRecoveryPoint needs
> to be updated when in recovery. That's a better definition for the call
> in bufmgr.c too.
>
> - I went ahead with the changes with RecoveryInfoLock and tracking the
> number of held AccessExclusive locks in lmgr.c instead of proc array.
>
> Can we find a better name for "loggable locks"? It means locks that need
> to be WAL logged when acquired, for hot standby, and "loggable lock"
> doesn't sound right for that. "Loggable" implies that it can be logged,
> but it really means that it *must* be logged.

The distinction of "loggable" is somewhat false since we rely on the
fact that only one person is holding it. So we may as well just call em
what they are: AccessExclusiveLocks.

> Keep an eye on my git repository for latest changes.

No, I'm not doing that. If you want to submit changes, please do so to
the list or just mention what needs work and I will do it. Having
multiple versions of a patch isn't helpful, as I have already said. I
have already been burned multiple times by accepting trial code and you
yourself have re-written particular parts multiple times. I am very,
very grateful for your reviews and detailed suggestions, so this comment
is only about coherency and not tripping each other up. (If you want to
"editorialize", it needs to be just prior to commit, but making changes
to a patch just prior to commit has historically shown to introduce bugs
where there weren't any before).

There's enough changes already to demand a full re-test, so everything
discovered so far plus testing is about 2 weeks work. I will commit
things onto git as agreed as I complete coding but that won't imply full
testing.

--
Simon Riggs www.2ndQuadrant.com