Re: [HACKERS] Hot standby, recovery procs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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.5070...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
On Tue, 2009-02-24 at 23:41 +, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot standby, recovery procs
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
