Re: Hot standby, slot ids and stuff

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