[SERVER-39489] Clarify optime creation in TransactionParticipant Created: 11/Feb/19 Updated: 29/Oct/23 Resolved: 23/May/19 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Replication |
| Affects Version/s: | None |
| Fix Version/s: | 4.1.12 |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Judah Schvimer | Assignee: | Judah Schvimer |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | prepare_durability | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||
| Sprint: | Repl 2019-05-20, Repl 2019-06-03 | ||||||||
| Participants: | |||||||||
| Description |
|
There is a line here that is no longer 100% true:
This is no longer true for prepared transactions since they can survive term changes. I expect this is still safe since prepared transactions will do a write and not wait on the _speculativeTransactionReadOpTime but this should be double checked, and commented as such. "OpTime creation" in the transaction participant is done here as well, very similarly. |
| Comments |
| Comment by Githook User [ 23/May/19 ] |
|
Author: {'name': 'Judah Schvimer', 'email': 'judah@mongodb.com', 'username': 'judahschvimer'}Message: |
| Comment by Esha Maharishi (Inactive) [ 15/May/19 ] |
|
tess.avitabile, I didn't read all the above comments, but we are not planning to do that optimization before 4.2. I created a ticket for it (SERVER-41165) and marked it as related to this ticket so it's easier to follow this paper trail if or when we do do it. |
| Comment by Judah Schvimer [ 15/May/19 ] |
|
Given that it's safe as matthew.russotto explained, I'll just improve the comment and maybe clean up the code a bit to keep the optimization easy. |
| Comment by Tess Avitabile (Inactive) [ 15/May/19 ] |
|
judah.schvimer, I agree that we can remove speculativeTransactionReadOptime completely, since we do a noop write for all read-only transactions, including transactions with readConcern level snapshot. My only concern is whether esha.maharishi planned to make an optimization to remove the noop write for read-only transactions with readConcern level snapshot before 4.2. |
| Comment by Judah Schvimer [ 15/May/19 ] |
|
matthew.russotto, Thanks! I agree. That said, do you agree that this is dead code now that we write noops for read-only transactions? |
| Comment by Matthew Russotto [ 15/May/19 ] |
|
That particular comment is only saying that the _speculativeTransactionReadOpTime is coherent; that is, that the term didn't change between the time we determined the read timestamp (with preallocateSnapshot() ) and the time we called getTerm(). At the time the fact that the term didn't change during a transaction was the only guarantee, but we have a stronger guarantee of that now: This method is called with the global and RSTL lock held, so term cannot change. |
| Comment by Judah Schvimer [ 15/May/19 ] |
|
OpTime creation first began in 4.0 in In 4.0, the comment associated with the OpTime creation was true “Transactions do not survive term changes, so combining "getTerm" here with the recovery unit timestamp does not cause races.”. This is no longer true due to prepared transactions surviving term changes. The next evolution in 4.0 was in At the beginning of 4.2, we found a bug fixed in This code evolved in 4.2 with These noop writes became related, however, in As a result, it should be safe to remove the speculativeTransactionReadOptime completely. We currently use it for logging, but the read timestamp goes in the SingleTransactionStats as well, which is the proper place for that information. The testing added in |
| Comment by Gregory McKeon (Inactive) [ 25/Mar/19 ] |
|
judah.schvimer can we get a prepare label on this? |