[SERVER-34664] Commands that are unsupported in a transaction should error Created: 25/Apr/18 Updated: 29/Oct/23 Resolved: 27/Jul/18 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Replication, Shell |
| Affects Version/s: | 3.7.6 |
| Fix Version/s: | 4.0.2, 4.1.2 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Shane Harvey | Assignee: | Tess Avitabile (Inactive) |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | documentation-needed | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Backwards Compatibility: | Minor Change | ||||||||||||||||
| Operating System: | ALL | ||||||||||||||||
| Backport Requested: |
v4.0
|
||||||||||||||||
| Sprint: | Repl 2018-07-02, Repl 2018-07-16, Repl 2018-07-30 | ||||||||||||||||
| Participants: | |||||||||||||||||
| Linked BF Score: | 0 | ||||||||||||||||
| Description |
|
This isMaster and ping commands (and possibly others) are not supported in a transaction but the server does not report an error when they are run within one.
Expected behavior: the isMaster should error because it's not supported in a transaction. For example:
|
| Comments |
| Comment by Githook User [ 06/Aug/18 ] |
|
Author: {'username': 'tessavitabile', 'name': 'Tess Avitabile', 'email': 'tess.avitabile@mongodb.com'}Message: (cherry picked from commit d56d941fe6d3d0af27ef8b8524e240e750a81364) |
| Comment by Githook User [ 27/Jul/18 ] |
|
Author: {'username': 'tessavitabile', 'name': 'Tess Avitabile', 'email': 'tess.avitabile@mongodb.com'}Message: |
| Comment by Githook User [ 27/Jul/18 ] |
|
Author: {'username': 'tessavitabile', 'name': 'Tess Avitabile', 'email': 'tess.avitabile@mongodb.com'}Message: |
| Comment by Tess Avitabile (Inactive) [ 18/Jun/18 ] |
|
This occurs because we do not parse the session/transaction command parameters for commands that do not require auth. Affected commands are:
|
| Comment by A. Jesse Jiryu Davis [ 06/Jun/18 ] |
|
I wouldn't put much effort into fixing the behavior of "isMaster" in particular; drivers themselves don't call it in a transaction, and if user applications call it in a transaction, for some reason, then inconsistencies in its behavior are low-impact. For commands in general, however, I agree that if a command is not fully supported in transactions then it should always error in a transaction. |
| Comment by Spencer Brody (Inactive) [ 05/Jun/18 ] |
|
jesse shane.harvey, I'd like to re-open this discussion. We always intended to error if we receive a command with a txnNumber when the command isn't supported in transactions, but it looks like we didn't actually get that check working right. Leaving this as-is results in some weird behavioral inconsistencies. Namely some commands (such as isMaster) are okay to be sent within a transaction if there have already been ops in the transaction, but will cause an error if they are the first operation within the transaction. It'd be better for the behavior to be consistent, and I think the right consistent behavior is for them to fail. Is there any objection to the server making this change? jesse shane.harvey? |
| Comment by A. Jesse Jiryu Davis [ 25/Apr/18 ] |
|
SGTM, thanks for thinking this through. |
| Comment by Shane Harvey [ 25/Apr/18 ] |
|
The problem here is that the client thinks it started a transaction because it ran a command with all the necessary transaction parameters ('isMaster' in this case). When the client runs the commitTransaction command it finds out that the server never started a transaction. This is surprising. That said I think I'm convinced that this doesn't need changes because banning 'isMaster' would be a little odd. As you said, they're not transactional. Perhaps we just need to clearly document the three types of commands:
jesse what do you think? |
| Comment by Eric Milkie [ 25/Apr/18 ] |
|
One thing we could do here is make it an error in the shell if you attempt to use the commitTransaction() helper without having first done any transactional commands in that session after a startTransaction() call. If we wanted to test the behavior of the commitTransaction server command in that situation, we would simply have to run that command manually without the helper, in the test. |
| Comment by Kyle Suarez [ 25/Apr/18 ] |
|
The drivers transactions spec doesn't mention that it's explicitly illegal to run those commands in a transaction, so I don't understand why we'd have to ban them. Like Eric said, they're not transactional; they're more like info commands. |
| Comment by Eric Milkie [ 25/Apr/18 ] |
|
I think those commands are not transactional, so they will work just fine regardless of whether you have told the shell you want to start a new transaction in your session. Doesn't the shell give you that error for "commitTransaction()" because you didn't do any transactional operations beforehand? I feel like you would get the same error if you did your repro but left out the part where you ran "ismaster". |