[SERVER-39627] Audit uses of TransactionParticipant::SideTransactionBlock Created: 15/Feb/19 Updated: 17/Jun/19 Resolved: 17/Jun/19 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Replication |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | A. Jesse Jiryu Davis | Assignee: | Judah Schvimer |
| Resolution: | Won't Fix | Votes: | 0 |
| Labels: | prepare_basic | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||||||
| Issue Links: |
|
||||||||||||||||
| Backport Requested: |
v4.2
|
||||||||||||||||
| Sprint: | Repl 2019-06-03, Repl 2019-06-17, Repl 2019-07-01 | ||||||||||||||||
| Participants: | |||||||||||||||||
| Description |
|
We use SideTransactionBlock to do things outside a transaction, while in a multi-document transaction. E.g., when OpObserverImpl writes the prepare oplog entry or (in It seems error-prone that a side transaction block only partly masks the state of the outer transaction. For example, state methods like TransactionParticipant::inMultiDocumentTransaction() reflect the outer transaction. This can cause mistakes when recursing into the TransactionParticipant while doing an operation in a side transaction via
Audit uses of SideTransactionBlock and consider extending the SideTransactionBlock to more thoroughly mask evidence of the TransactionParticipant's outer transaction. |
| Comments |
| Comment by Siyuan Zhou [ 14/Jun/19 ] | ||||||||||||||
|
Great analysis, judah.schvimer! I agreed the original concern by Jesse should be resolved using UnreplicatedWritesBlock, since missing UnreplicatedWritesBlock when it's needed sounds a more fundamental issue anyway. Doing nothing makes sense to me. | ||||||||||||||
| Comment by Andy Schwerin [ 14/Jun/19 ] | ||||||||||||||
|
SGTM | ||||||||||||||
| Comment by Judah Schvimer [ 14/Jun/19 ] | ||||||||||||||
|
I'm proposing to do nothing in this ticket. There is definitely still a chance of deadlock using the ACR improperly, but I think for now it makes more sense to fix those one by one. If someone wants to advocate for finishing the POC of having ACRs inherit lockers, I'm open to it, but do not want to invest the time right now to prove this is safe given siyuan.zhou's concern. I will close this ticket "Won't Fix" in a few days if I hear nothing else, though we can always reopen it. | ||||||||||||||
| Comment by Judah Schvimer [ 12/Jun/19 ] | ||||||||||||||
Thus I do not see any work to do now based on this audit. | ||||||||||||||
| Comment by Judah Schvimer [ 12/Jun/19 ] | ||||||||||||||
|
The original problem this ticket was addressing is hinted at at the bottom of this cr on line 2148. An older version of The goal of this ticket is to replace STB with ACR. To fully do this we would need to give ACR similar treatment to This implies two separate changes:
There are three usages of STB today:
It seems to me that basically all of the concerns are around making the ACR share its parent locker. Andy is concerned about currentOp, killOp, and killSession. Siyuan is concerned about the fundamental assumptions here. As Tess said though, sharing the locker is important in terms of preventing deadlocks. This seems like the only real motivating purpose of this change right now. Siyuan's concern is valid and I personally do not know what could go wrong by breaking the 1:1 mapping from client:locker. To address Andy's concern we'll go through the three concerning commands and how the two changes suggested would impact them:
The biggest change appears to be the lock stats becoming more accurate, which would be a win. This ends up being a mixed bag. I'd love for some outside input on Siyuan's more general concern. In the meantime I will actually do the brief audit this ticket requested. | ||||||||||||||
| Comment by Siyuan Zhou [ 05/Jun/19 ] | ||||||||||||||
|
I think the existing one to one mapping among Client -> OperationContext -> Locker is a fundamental assumption. Conceptually, it's not clear to me why side transaction has to mask all states of the running operation context as suggested by this ticket. I feel there are more states we want to keep than stash/mask. Locker is probably not the only. The statistics of the operation, session checkout and whether the operation should conflict with PBWM lock are also bound to operation context that I can think of off the top of my head. Client seems too high-level to isolate the resources here. At the code level, when working on | ||||||||||||||
| Comment by Eric Milkie [ 05/Jun/19 ] | ||||||||||||||
|
The code above I wrote for a section that intended to be a handler for an op being killed; the reason for using a new Client was to clear out the current killCode attached to the originalOpCtx. If one doesn't do this, no further db locking is possible in an abort handler since the lock would check the kill code and immediately rethrow an Interrupted exception. | ||||||||||||||
| Comment by Judah Schvimer [ 05/Jun/19 ] | ||||||||||||||
|
milkie may have thought about that. I need to do further investigation and testing to find out. | ||||||||||||||
| Comment by Andy Schwerin [ 05/Jun/19 ] | ||||||||||||||
|
What does that do for currentOp, killOp and killSessions? | ||||||||||||||
| Comment by Judah Schvimer [ 05/Jun/19 ] | ||||||||||||||
|
milkie provided the following which works for a given ACR (though doesn't work in the change from
| ||||||||||||||
| Comment by Judah Schvimer [ 31/May/19 ] | ||||||||||||||
|
I don't think a PoC would be too hard. I'll do it next iteration. I knew this ticket might be bigger than originally expected. | ||||||||||||||
| Comment by Andy Schwerin [ 30/May/19 ] | ||||||||||||||
|
I'm definitely interested in the PoC. We'd need to think about how current-op reporting would look. All the work in the ACR gets assigned to a different client and op context, which might affect metrics. | ||||||||||||||
| Comment by Tess Avitabile (Inactive) [ 30/May/19 ] | ||||||||||||||
|
On the other hand, having an ACR that does not share the locker is prone to self-deadlock. | ||||||||||||||
| Comment by Andy Schwerin [ 30/May/19 ] | ||||||||||||||
|
I'd look at a PoC, but having an ACR where the alternate client shares OpCtx state (the locker) with the principal client's OpCtx seems error-prone, too. How hard to poc, judah.schvimer? | ||||||||||||||
| Comment by Judah Schvimer [ 29/May/19 ] | ||||||||||||||
|
One concern with the above plan is that it somewhat is a layering violation between the Locker and the Client. The Client would have to release and restore the OperationContext's WUOW similar to in siyuan.zhou, schwerin, geert.bosch, do any of you have any thoughts on this? I can create a POC if that seems helpful. | ||||||||||||||
| Comment by Tess Avitabile (Inactive) [ 29/May/19 ] | ||||||||||||||
|
judah.schvimer and I discussed the following strategy for SideTransactionBlock. We could get rid of SideTransactionBlock entirely and use AlternativeClientRegion everywhere instead. This would have the advantage that the readConcern on the OperationContext and the TransactionParticipant would both be fresh, so code that checks these constructs does not need to consider whether we are in a SideTransactionBlock. Due to As part of this work, we could eliminate checks of OperationContext::getWriteUnitOfWork(), since this should now return the same result as TransactionParticipant::inMultiDocumentTransaction(). If this leads to linking issues, we could consider adding an OperationContext flag/decoration that indicates whether the operation is a statement of a multi-document transaction (i.e. it has autocommit:false). This might be preferable anyway, since most users probably expect this to be the semantics of TransactionParticipant::inMultiDocumentTransaction(), when in fact that function returns false once the transaction is aborted/committed. | ||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 22/Feb/19 ] | ||||||||||||||
|
I attached a patch atop git version #2c65bbe9, it's a sketch of one direction we could go to fix the reentrance problem. | ||||||||||||||
| Comment by Judah Schvimer [ 15/Feb/19 ] | ||||||||||||||
|
blake.oler, I could imagine your work looking at the transaction statements at commit time being impacted by this. The prepare oplog entry or the config.transactions write may be added to the operations list in the prepare oplog entry SideTransactionBlock. I have not confirmed this, however. |