Re: Hot standby, slot ids and stuff

Heikki, can I get your feedback on this urgently please? I want to
respond positively to your review comments and complete something you
will find acceptable. But I need to know what that is, please.

On Sun, 2009-01-11 at 11:55 +0000, Simon Riggs wrote:
> On Sun, 2009-01-11 at 10:41 +0200, Heikki Linnakangas wrote:
> > Simon Riggs wrote:
> > > There's a confusion in the patch between top level xid and parent xid.
> > > The xl field is named parentxid but actually contains top level. That
> > > distinction is important because the code now uses the top level xid to
> > > locate the recovery proc, formerly the role of the slotid.
> >
> > True, I changed the meaning halfway through. My thinking was that we can
> > get away by only tracking the top-level xact of each subtransaction, not
> > the whole tree of subtransactions.
> >
> > > This leads to an error when we SubTransSetParent(child_xid, top_xid);
> > > since this assumes that the top_xid is the parent, which it is not.
> > > Mostly you wouldn't notice unless you were looking up the subtrans
> > > status for an xid that had committed but was the child of an aborted
> > > subtransaction, with the top level xid having > 64 subtransactions.
> >
> > Hmm. When a subtransaction aborts, RecordTransactionAbort is called and
> > clog is updated to show the subtransaction and all it's subcommitted
> > children as aborted. I think we're safe, though it wouldn't hurt to
> > double-check.
>
> That part of your argument is correct but we are not safe. But please
> look at XactLockTableWait() and see what you think. According to
> comments this would lead to different lock release behaviour.
>
> Beauty, thy name is not subtransaction.
>
> If you agree that we need the parent xid then we have further problems.
> Adding another xid onto the header of each WAL record will add 8 bytes
> onto each record because of alignment. This was the other reason for
> slotid, since I was able to fit that into just 2 bytes and avoid the 8
> byte loss. Moving swiftly on, I have this morning though of an
> alternative, so at least we now have some choice. Rather than store the
> parent xid itself we store the difference between the current xid and
> the parent xid. Typically this will be less than 65535; when it is not
> we set it to zero but issue an xid assignment xlog record.
>
> However, I think XactLockTableWait() doesn't need to know the parent
> either. (This feels more like wishful thinking, but here goes anyway).
> We release locks *after* TransactionIdAbortTree() has fully executed, so
> the test for TransactionIdIsInProgress(xid) will always see the abort
> status, if set. Notice that if we are awake at all it is because the
> top-level transaction is complete or our subxid is aborted. So there is
> never any need to look at the status of the parent, nor in fact any need
> to look at the procarray at all, which is always a waste of effort. If
> you believe that, you'll want to commit the attached patch (or something
> similar with comments refactored etc).
>
> > For xl_rel_lock you add a field called xid and then ask
> > > /* xid of the *parent* transaction. XXX why parent? */.
> > > You've done this because it replaced slotid. But looking at that, I
> > > think the 6a patch had a bug there: a subtransaction abort record would
> > > release locks for the whole top level xact. So we need to pass both top
> > > level xid (or slotid) and xid for each lock, then release using the
> > > actual xid only.
> >
> > Hmm, would just the subtransaction xid be enough? If we use that to
> > release, what do we need the top-level xid for?
>
> So we can release all locks taken by subxids at once, when we learn that
> a top level xid has disappeared. A minor point.
>
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support