Re: Hot standby, recovery procs

(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.

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

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. 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.

Those objects live and die completely differently to unobservedxids,
which don't need (nor can they have) the more complex data structure.

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.

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

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

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

On Tue, 2009-02-24 at 21:59 +0200, Heikki Linnakangas wrote:
> 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.

No, because we check whether TransactionIdDidCommit().

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Tue, 2009-02-24 at 21:59 +0200, Heikki Linnakangas wrote:
>> 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.
>
> No, because we check whether TransactionIdDidCommit().

Oh, right... But we have the same problem with the subtransactions,
don't we? This block:

> /*
> * If our state information is later for this proc, then
> * overwrite it. It's possible for a commit and possibly
> * a new transaction record to have arrived in WAL in between
> * us doing GetRunningTransactionData() and grabbing the
> * WALInsertLock, so we musn't assume we always know best.
> */
> if (XLByteLT(proc->lsn, lsn))
> {
> TransactionId *subxip = (TransactionId *) &(xlrec->xrun[xlrec->xcnt]);
>
> proc->lsn = lsn;
> /* proc-> pid stays 0 for Recovery Procs */
>
> proc->subxids.nxids = rxact[xid_index].nsubxids;
> proc->subxids.overflowed = rxact[xid_index].overflowed;
>
> memcpy(proc->subxids.xids, subxip,
> rxact[xid_index].nsubxids * sizeof(TransactionId));
>
> /* Remove subtransactions from UnobservedXids also */
> if (unobserved)
> {
> for (index = 0; index < rxact[xid_index].nsubxids; index++)
> UnobservedTransactionsRemoveXid(subxip[index + rxact[xid_index].subx_offset], false);
> }
> }

overwrites subxids array, and will resurrect any already aborted
subtransaction.

Isn't XLByteLT(proc->lsn, lsn) always true, because 'lsn' is the lsn of
the WAL record we're redoing, so there can't be any procs with an LSN
higher than that?

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

On Tue, 2009-02-24 at 21:59 +0200, Heikki Linnakangas wrote:

> > 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.

Last week, I think. Why are these tweaks so important?

Checking pg_subtrans for every call to XidInMVCCSnapshot will destroy
performance, as well you know.

> > 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.

It might and it might not, but I don't believe all angles have been
evaluated. But I would say that major changes such as this have resulted
in weeks of work. More bugs have been introduced since feature freeze
than were present beforehand.

If you want this code to fail, then twisting it in lots of directions
every week is exactly the way to do that. Neither of us will understand
how it works and we'll take more weeks for it to settle down to the
point of reviewability again. We don't have weeks any more.

So far I've made every change you've asked, but there is a reasonable
limit.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Tue, 2009-02-24 at 22:29 +0200, Heikki Linnakangas wrote:

> Oh, right... But we have the same problem with the subtransactions,
> don't we? This block:
>
> > /*
> > * If our state information is later for this proc, then
> > * overwrite it. It's possible for a commit and possibly
> > * a new transaction record to have arrived in WAL in between
> > * us doing GetRunningTransactionData() and grabbing the
> > * WALInsertLock, so we musn't assume we always know best.
> > */
> > if (XLByteLT(proc->lsn, lsn))
> > {
> > TransactionId *subxip = (TransactionId *) &(xlrec->xrun[xlrec->xcnt]);
> >
> > proc->lsn = lsn;
> > /* proc-> pid stays 0 for Recovery Procs */
> >
> > proc->subxids.nxids = rxact[xid_index].nsubxids;
> > proc->subxids.overflowed = rxact[xid_index].overflowed;
> >
> > memcpy(proc->subxids.xids, subxip,
> > rxact[xid_index].nsubxids * sizeof(TransactionId));
> >
> > /* Remove subtransactions from UnobservedXids also */
> > if (unobserved)
> > {
> > for (index = 0; index < rxact[xid_index].nsubxids; index++)
> > UnobservedTransactionsRemoveXid(subxip[index + rxact[xid_index].subx_offset], false);
> > }
> > }
>
> overwrites subxids array, and will resurrect any already aborted
> subtransaction.
>
> Isn't XLByteLT(proc->lsn, lsn) always true, because 'lsn' is the lsn of
> the WAL record we're redoing, so there can't be any procs with an LSN
> higher than that?

I'm wondering whether we need those circumstances at all.

The main role of ProcArrayUpdateRecoveryTransactions() is two-fold
* initialise snapshot when there isn't one
* reduce possibility of FATAL errors that don't write abort records

Neither of those needs us to update the subxid cache, so we'd be better
off avoiding that altogether in the common case. So we should be able to
ignore the lsn and race conditions altogether.

It might even be more helpful to explicitly separate those twin roles so
the code is clearer.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Tue, 2009-02-24 at 23:41 +0000, Simon Riggs wrote:
> On Tue, 2009-02-24 at 22:29 +0200, Heikki Linnakangas wrote:

> > overwrites subxids array, and will resurrect any already aborted
> > subtransaction.
> >
> > Isn't XLByteLT(proc->lsn, lsn) always true, because 'lsn' is the lsn of
> > the WAL record we're redoing, so there can't be any procs with an LSN
> > higher than that?
>
> I'm wondering whether we need those circumstances at all.
>
> The main role of ProcArrayUpdateRecoveryTransactions() is two-fold
> * initialise snapshot when there isn't one
> * reduce possibility of FATAL errors that don't write abort records
>
> Neither of those needs us to update the subxid cache, so we'd be better
> off avoiding that altogether in the common case. So we should be able to
> ignore the lsn and race conditions altogether.

We still have a race condition for the initial snapshot, so your concern
still holds. Thanks for highlighting it.

I'm in the middle of rewriting ProcArrayUpdateRecoveryTransactions() to
avoid errors caused by these race conditions. The LSN flag was an
attempt to do that, but was insufficient and has now been removed.

I'll discuss it more when I've got it working. Seems like we need
working code now rather than lengthy debates. I see a solution and
almost have it done.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Tue, 2009-02-24 at 21:59 +0200, Heikki Linnakangas wrote:
>>> 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.
>
> It might and it might not, but I don't believe all angles have been
> evaluated. But I would say that major changes such as this have resulted
> in weeks of work. More bugs have been introduced since feature freeze
> than were present beforehand.

Here's a rough sketch of how the transaction tracking could work without
recovery procs, relying on unobserved xids array only. The "unobserved
xids" is a complete misnomer now, as it tracks all master-transactions,
and there's no distinction between observed and unobserved ones.

Another big change in this patch is the way xl_xact_assignment records
work. Instead of issuing one such WAL record for each subtransaction
when they're being assigned recursively, we keep track of which xids
have already been "reported" in the WAL (similar to what you had in an
earlier version of the patch). Whenever you hit the limit of 64
unreported subxids, you issue a single WAL record listing all the
unreported subxids of this top-level transactions, and mark them as
reported. The limit of 64 is chosen arbitrarily, but it should match the
number of slots in the unobserved xids array per backend, to avoid
running out of slots. This eliminates the need for the xl_topxid field
in the WAL record header. I think one WAL record per 64 assigned
subtransactions is a small price to pay, considering that a transaction
with that many subtransactions is probably doing some interesting work
anyway, and the volume of those assignment WAL records is lost in the
noise of all the other WAL records the transactions issues.

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

Forgot attachment, here it is as a patch against CVS HEAD. It's also in
my git repository.

Heikki Linnakangas wrote:
> Simon Riggs wrote:
>> On Tue, 2009-02-24 at 21:59 +0200, Heikki Linnakangas wrote:
>>>> 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.
>>
>> It might and it might not, but I don't believe all angles have been
>> evaluated. But I would say that major changes such as this have resulted
>> in weeks of work. More bugs have been introduced since feature freeze
>> than were present beforehand.
>
> Here's a rough sketch of how the transaction tracking could work without
> recovery procs, relying on unobserved xids array only. The "unobserved
> xids" is a complete misnomer now, as it tracks all master-transactions,
> and there's no distinction between observed and unobserved ones.
>
> Another big change in this patch is the way xl_xact_assignment records
> work. Instead of issuing one such WAL record for each subtransaction
> when they're being assigned recursively, we keep track of which xids
> have already been "reported" in the WAL (similar to what you had in an
> earlier version of the patch). Whenever you hit the limit of 64
> unreported subxids, you issue a single WAL record listing all the
> unreported subxids of this top-level transactions, and mark them as
> reported. The limit of 64 is chosen arbitrarily, but it should match the
> number of slots in the unobserved xids array per backend, to avoid
> running out of slots. This eliminates the need for the xl_topxid field
> in the WAL record header. I think one WAL record per 64 assigned
> subtransactions is a small price to pay, considering that a transaction
> with that many subtransactions is probably doing some interesting work
> anyway, and the volume of those assignment WAL records is lost in the
> noise of all the other WAL records the transactions issues.
>

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

Simon Riggs wrote:
> On Tue, 2009-02-24 at 21:59 +0200, Heikki Linnakangas wrote:
>
>>> 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.
>
> Last week, I think. Why are these tweaks so important?

Heh, actually, I went searching my mail for when I had suggested that,
and found that in fact I proposed this exact same method of using the
unobserved xids array only back in October:

http://archives.postgresql.org/message-id/48F76342.5070407@enterprisedb.com

I had since forgotten all about, but now came up with the same idea
again during review.

In the first reply in that thread you said that "The main problem is
fatal errors that don't write abort records. By reusing the PROC entries
we can keep those to a manageable limit". We're not worried about that
anymore.

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

On Thu, 2009-02-26 at 10:04 +0200, Heikki Linnakangas wrote:

> we keep track of which xids
> have already been "reported" in the WAL (similar to what you had in an
> earlier version of the patch)

You objected to doing exactly that earlier. Why is it OK to do it now
that you are proposing it?

You haven't even given a good reason to make these changes.

We don't have time to make this change and then shake out everything
else that will break as a result. Are you suggesting that you will make
these changes and then follow up on all other breakages? Forcing this
request seems like a great way to cancel this patch, since it will be
marked as "author refused to make change".

You have spotted a problem elsewhere and I am working to fix that now.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Thu, 2009-02-26 at 10:04 +0200, Heikki Linnakangas wrote:
>
>> we keep track of which xids
>> have already been "reported" in the WAL (similar to what you had in an
>> earlier version of the patch)
>
> You objected to doing exactly that earlier.

I'm talking about the "xidMarkedInWAL" and "hasUnMarkedSubXids" fields
you had in TransactionState, at least still in version
hs.v7.20090112_1.tar.bz2 of the patch. I objected to adding the
corresponding flags in the WAL header, and that made tracking the status
in TransactionState obsolete in the patch too, since it wasn't used for
anything anymore. There's nothing wrong per se about tracking the
"marked" or "reported" status in master.

> You haven't even given a good reason to make these changes.

Simplicity.

> We don't have time to make this change and then shake out everything
> else that will break as a result. Are you suggesting that you will make
> these changes and then follow up on all other breakages? Forcing this
> request seems like a great way to cancel this patch, since it will be
> marked as "author refused to make change".

I'm not suggesting anything to be canceled. I simply think these are
changes that should be made. I wish you could make them, because that
means less work for me. But if you're not willing to, I can pick it up
myself.

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

On Thu, 2009-02-26 at 11:36 +0200, Heikki Linnakangas wrote:

> > You haven't even given a good reason to make these changes.
>
> Simplicity.

You used that argument in January to explain why the coupling should be
reduced and now the same argument to put it back again.

> > We don't have time to make this change and then shake out everything
> > else that will break as a result. Are you suggesting that you will make
> > these changes and then follow up on all other breakages? Forcing this
> > request seems like a great way to cancel this patch, since it will be
> > marked as "author refused to make change".
>
> I'm not suggesting anything to be canceled. I simply think these are
> changes that should be made. I wish you could make them, because that
> means less work for me. But if you're not willing to, I can pick it up
> myself.

When you review my code, you make many useful suggestions and I am very
thankful. Testing can't find out some of those things. My feeling is
that you are now concentrating on things that are optional, yet will
have a huge potential for negative impact. If I could please draw your
review efforts to other parts of the patch, I would be happy to return
to these parts later.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Thu, 2009-02-26 at 11:36 +0200, Heikki Linnakangas wrote:
>
>>> You haven't even given a good reason to make these changes.
>> Simplicity.
>
> You used that argument in January to explain why the coupling should be
> reduced and now the same argument to put it back again.

That was in reference to the slot ids, I'm not suggesting to put that
back. If anything, removing the need for the the xl_topxid field in WAL
record will further reduce the coupling between master and standby.

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

On Thu, 2009-02-26 at 12:19 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > On Thu, 2009-02-26 at 11:36 +0200, Heikki Linnakangas wrote:
> >
> >>> You haven't even given a good reason to make these changes.
> >> Simplicity.
> >
> > You used that argument in January to explain why the coupling should be
> > reduced and now the same argument to put it back again.
>
> That was in reference to the slot ids, I'm not suggesting to put that
> back. If anything, removing the need for the the xl_topxid field in WAL
> record will further reduce the coupling between master and standby.

OK, well, if you feel those changes are necessary prior to commit then I
would ask you do that in your public repo and we'll test and provide
helpful comments on it from there as quickly as we can. Too many cooks
spoil the git.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support