[DRIVERS-2172] db and coll handles write concern and read concern are silently ignored in transactions Created: 13/Mar/19 Updated: 31/Mar/22 |
|
| Status: | Backlog |
| Project: | Drivers |
| Component/s: | Transactions |
| Fix Version/s: | None |
| Type: | Spec Change | Priority: | Major - P3 |
| Reporter: | Henrik Ingo (Inactive) | Assignee: | Unassigned |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Driver Changes: | Needed | ||||||||||||
| Description |
|
As elaborated in PYTHON-1668, it turns out that if I set write_concern or read_concern for the db or collection handles, they will be silently ignored. If I understand the background to this situation correctly: A transaction can mix several db and coll handles in a single transaction. They can have different write_concerns and read_concerns, yet the transaction obviously can only have one and the same, as those are properties of a transaction. The solution is to silently ignore those settings for db and coll objects inside a transaction. For users who won't read the documentation, this is not obvious and I fear many will not get the durability and consistency they expected. (Just like I didn't.) This is essentially a data corruption bug - even if a UX kind of that. I would suggest the following instead: db and coll objects inside a transaction MUST have write_concern and read_concern set to either:
|
| Comments |
| Comment by Henrik Ingo (Inactive) [ 09/Apr/19 ] | ||||||||||||||||||||||||
|
Hi Jeremy
It was undefined.
I realize this may not be trivial to implement for all drivers. Still, we can not just silently ignore consistency levels because it is convenient. This causes data corruption to users.
If I'm following correctly, the client concerns will also be inherited to the session, which will be inherited to the transaction. So if the user only sets the concerns via the client, then both of the below are true and there is no error:
No, because write concern was not set on either of db or collection. That said, your example does raise another interesting point. The dangerous case is really when start_transaction() is created without explicit concerns, because it is not obvious what it inherits:
So the interesting question is what to do in this case:
You could argue that in this case there is no risk for misunderstanding, so no error is needed. The counter arguments would be:
| ||||||||||||||||||||||||
| Comment by Jeremy Mikola [ 08/Apr/19 ] | ||||||||||||||||||||||||
Not sure if "users create db and collection objects without specifying a concern" was implying that the Client also had no RC/WC specified. If not, I think it bears mentioning that some drivers may not differentiate between whether a WC/RC on a Database or Collection object was inherited (e.g. from the Client) or explicitly set by the user. Consider: mongoc_client_get_collection in libmongoc, which always inherit the Client's options. The RC/WC on the return Collection can then be altered via corresponding setter methods.
Given the above, where Database and Collection objects always inherit from the Client, it may not be possible for users to create these objects without a RC/WC if the original connection string included an explicit value. Question for henrik.ingo and scott.lhommedieu: would you expect the following to error as well?
| ||||||||||||||||||||||||
| Comment by Scott L'Hommedieu (Inactive) [ 08/Apr/19 ] | ||||||||||||||||||||||||
|
shane.harvey in re: to
I'm suggesting that the ignores portion be changed to error if not equal. | ||||||||||||||||||||||||
| Comment by Henrik Ingo (Inactive) [ 08/Apr/19 ] | ||||||||||||||||||||||||
|
Thanks Scott
Note that for most users - using python, javascript, etc - this is normal behavior for that language. For C++, Java, etc... In many cases these settings may be supplied as runtime configurations - such as in the URI - so it's not possible to know at compile time if they are correct. Shane:
Note that your example doesn't match what I have proposed in this ticket. If users create db and collection objects without specifying a concern, then this is fine. An error should however be raised when users explicitly set a concern that can't be used in the transaction. Silently ignoring something the user explicitly asked for is not the right solution. | ||||||||||||||||||||||||
| Comment by Shane Harvey [ 05/Apr/19 ] | ||||||||||||||||||||||||
|
> But, could the insert_one(..., session=s) do the get_txn_x_concern work without requiring the user to do it. That is what we have now. insert_one(..., session=s) runs the insert as part of a transaction and ignores the read/write concern of the collection. | ||||||||||||||||||||||||
| Comment by Scott L'Hommedieu (Inactive) [ 05/Apr/19 ] | ||||||||||||||||||||||||
|
Thanks for the details, shane.harvey I hadn't gotten into details on the implementation. I want to make sure this behavioral (how the code should be written) and functional (how ops work in txn) change is well conveyed to developers. We've heard from developers that these types of changes can go unexplained or worse unnoticed causing havok on systems/data. I hate to get into these details but I'll see how this goes... I'm wondering, why do these db and coll functions require that users specify the write concern? Can you not determine the concern from available state (as set by being in the transaction)? It looks like maybe the get_collection doesn't have access to the state in order to set the colls concern to the same as the session/txn. But, could the insert_one(..., session=s) do the get_txn_x_concern work without requiring the user to do it. If the sessions and db/colls are != then error. | ||||||||||||||||||||||||
| Comment by Shane Harvey [ 05/Apr/19 ] | ||||||||||||||||||||||||
|
We decided to rely on the transaction's read/write concern and ignore the collection's settings because it is the most convenient api. scott.lhommedieu, henrik.ingo, consider that if we raise an error when a collection's setting is different from the transaction's then writing transactions "correctly" will become a very tedious and user-unfriendly exercise. Users would need to ensure all collection objects are configured with the same settings as the current transaction. So this kind of code:
Would need to become:
And it would be even more annoying to write a generic transaction meth that could be used with multiple transactions (thus multiple different read/write concerns). We would need add public methods to lookup the current transaction's settings:
I am strongly opposed to such a change. | ||||||||||||||||||||||||
| Comment by Bernie Hackett [ 05/Apr/19 ] | ||||||||||||||||||||||||
|
alyson.cabral, can we get your thoughts on this? Whatever decision is made will have to apply to the shell (and every other user interface) as well. | ||||||||||||||||||||||||
| Comment by Scott L'Hommedieu (Inactive) [ 05/Apr/19 ] | ||||||||||||||||||||||||
|
I advocate for the suggestion that henrik.ingo is making here. The user (developer) should be made aware that the concern they are using on an operation in a transaction isn't valid and WILL NOT be respected in execution of the operation. Because this will occur silently, relying on documentation for this will not provide an adequate level of guidance for the user to realize an error, understand the errors meaning and determine a resolution to the error. Unfortunately, this will result in a runtime error for something that might better be surfaced as a compile time error. But, that doesn't seem feasible to implement here. Better to have a runtime error than no error at all.
| ||||||||||||||||||||||||
| Comment by Jeremy Mikola [ 03/Apr/19 ] | ||||||||||||||||||||||||
|
Based on discussion in PYTHON-1668, I'm going to repurpose this to modify the transactions spec and require drivers to document this interaction at a minimum and we can consider requiring drivers to raise client-side errors if a transaction's RC/WC would override a DB or Collection-level value. After this SPEC ticket is implemented we can create a new DRIVERS ticket and spawn per-language tickets accordingly. |