Re: Hot standby, recovery procs

Simon Riggs wrote:
> On Tue, 2009-02-24 at 10:40 +0200, Heikki Linnakangas wrote:
>> (back to reviewing the main hot standby patch at last)
>>
>> Why do we need recovery procs? AFAICS the only fields that we use are
>> xid and the subxid cache. Now that we also have the unobserved xids
>> array, why don't we use it to track all transactions in the master, not 
>> just the unobserved ones.
> 
> We need an array of objects defined in shared memory that has a
> top-level xid and a subxid cache.

Not really. The other transactions, taking snapshots, don't need to 
distinguish top-level xids and subxids. That's why the unobserved xids 
array works to begin with. We only need a list of running 
(sub)transaction ids. Which is exactly what unobservedxids array is.

The startup process can track the parent-child relationships in private 
memory if it needs to. But I can't immediately see why it would need to: 
commit and abort records list all the subtransactions. To keep the 
unobserved xids array bounded, when we find out about a parent-child 
relationship, via an xact-assignment record or via the xid and top-level 
xid fields in other WAL records, we can simply use SubtransSetParent. To 
keep it real simple, we can stipulate that you always check subtrans in 
XidIdInMVCCSnapshot while in hot standby mode.

> That object also needs an lsn
> attribute. We need code that adds these, removes them and adds the data
> onto snapshots in almost identical ways to current procarray code.

We only need the lsn atrribute because we when we take the snapshot of 
running xids, we don't write it to the WAL immediately, and a new 
transaction might begin after that. If we close that gap in the master, 
we don't need the lsn in recovery procs.

Actually, I think the patch doesn't get that right as it stands:

0. Transactions 1 is running in master
1. Get list of running transactions
2. Transaction 1 commits.
3. List of running xacts is written to WAL

When the standby replays the xl_running_xacts record, it will create a 
recovery proc and mark the transaction as running again, even though it 
has already committed.

PS. This line in the same function (ProcArrayUpdateRecoveryTransactions) 
seems wrong as well:
>             memcpy(proc->subxids.xids, subxip, 
>                         rxact[xid_index].nsubxids * sizeof(TransactionId));

I don't think "subxip" is correct for the 2d argument.

> I think if I had not made those into procs you would have said that they
> are so similar it would aid code readability to have them be the same.

And in fact I suggested earlier that we get rid of the unobserved xids 
array, and only use recovery procs.

> What benefit would we gain from separating them, especially since we now
> have working, tested code?

Simplicity. That matters a lot. Removing the distinction between 
unobserved xids and already-observed running transactions would slash a 
lot of code.

I appreciate your testing, but it's not like it has gone through years 
of usage in the field. This is not the case of "if it ain't broken, 
don't fix it". The code that's in the patch is not in production yet, 
and now is precisely the right time to get it right, before it goes into 
the "if it ain't broke, don't fix it" mode.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com