[CDRIVER-580] Write concern with w=0 and j=true is a GLE op Created: 13/Mar/15 Updated: 08/Jan/24 Resolved: 06/Apr/15 |
|
| Status: | Closed |
| Project: | C Driver |
| Component/s: | None |
| Affects Version/s: | 1.1.2 |
| Fix Version/s: | 1.1.5 |
| Type: | Bug | Priority: | Minor - P4 |
| Reporter: | Jeremy Mikola | Assignee: | A. Jesse Jiryu Davis |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||
| Description |
|
Currently, _mongoc_write_concern_needs_gle() returns true only if w is neither 0 or -1. This function is used by both the legacy and write command code paths. If the user were to specify w:0 and j:true, the driver skips sending a GLE. Although an edge case, this is unfortunate in that the server would otherwise have upgraded that w:0 to w:1 in order to honor the j:true option. For write commands, if the user specifies any values for a write concern, they should be included in the command(s). I believe there are also implications for replSetGetConfig.settings.getLastErrorDefaults, as that option kicks in only when the driver does not specify a write concern. A fix will start with having the driver differentiate between false and unset values for journal and fsync_ on the write concern struct. From there, the logic for _mongoc_write_concern_needs_gle() and BSON serialization can be changed. |
| Comments |
| Comment by Githook User [ 07/Oct/15 ] |
|
Author: {u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}Message: Merge remote-tracking branch 'upstream/master' into 1.2.0-dev
Conflicts: |
| Comment by Githook User [ 07/Oct/15 ] |
|
Author: {u'username': u'jmikola', u'name': u'Jeremy Mikola', u'email': u'jmikola@gmail.com'}Message: This adds support for parsing the readPreference URI option. It also implements validation of the read preference and returns a NULL URI pointer on failure, similar to the write concern validation added by Unfortunately, mongoc_uri_get_read_prefs() was already taken for the API function that returns read preference tags from the URI. This commit adds a new function, mongoc_uri_get_read_prefs_t(), which returns the actual mongoc_read_prefs_t struct (now used by the client constructor). |
| Comment by Githook User [ 07/Apr/15 ] |
|
Author: {u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}Message: Merge remote-tracking branch 'upstream/master' into 1.2.0-dev
Conflicts: |
| Comment by Githook User [ 07/Apr/15 ] |
|
Author: {u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}Message: Merge branch 'pr-194'
|
| Comment by Githook User [ 07/Apr/15 ] |
|
Author: {u'username': u'jmikola', u'name': u'Jeremy Mikola', u'email': u'jmikola@gmail.com'}Message: The fsync and journal options should imply a GLE; however, they logically conflict with w=0 and w=-1, so the write concern should be considered invalid with errors raised during URI parsing or write command execution. If fsync or journal have been specified (true or false), they should be included in the write concern BSON. |
| Comment by Githook User [ 07/Apr/15 ] |
|
Author: {u'username': u'jmikola', u'name': u'Jeremy Mikola', u'email': u'jmikola@gmail.com'}Message: This adds support for parsing the readPreference URI option. It also implements validation of the read preference and returns a NULL URI pointer on failure, similar to the write concern validation added by Unfortunately, mongoc_uri_get_read_prefs() was already taken for the API function that returns read preference tags from the URI. This commit adds a new function, mongoc_uri_get_read_prefs_t(), which returns the actual mongoc_read_prefs_t struct (now used by the client constructor). |
| Comment by Githook User [ 06/Apr/15 ] |
|
Author: {u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}Message: Merge branch 'pr-194'
|
| Comment by Githook User [ 06/Apr/15 ] |
|
Author: {u'username': u'jmikola', u'name': u'Jeremy Mikola', u'email': u'jmikola@gmail.com'}Message: The fsync and journal options should imply a GLE; however, they logically conflict with w=0 and w=-1, so the write concern should be considered invalid with errors raised during URI parsing or write command execution. If fsync or journal have been specified (true or false), they should be included in the write concern BSON. |
| Comment by Jeremy Mikola [ 30/Mar/15 ] |
|
I believe the linked PR is ready to go. The last todo item (warning message suppression) was implemented last week. |
| Comment by Jeremy Mikola [ 20/Mar/15 ] |
|
PR is updated and includes test cases for reporting errors on an invalid write concern during both URI parsing and write command execution. There's an outstanding question on whether the warning emission is OK and how we might want to suppress or assert it during the test. While looking into the URI parsing, I noticed that invalid w values (i.e. anything less than -1) trigger a warning but are otherwise ignored. I opened |
| Comment by Jeremy Mikola [ 19/Mar/15 ] |
|
I spoke to mira.carey@mongodb.com about this earlier today. Asserts were a bad idea because it could unpleasantly abort the program and this is really more of a runtime error, since write concerns could be generated from dynamic input. Jason pointed out that _mongoc_write_concern_is_valid() is the common choke point for runtime usage of write concerns. I'm going to implement a _mongoc_write_concern_is_valid() function in the private API (akin to mongoc_read_prefs_is_valid()), which will check that we have a logically correct data structure. If not, we can report an error in the result structure, which is consistent with existing runtime error reporting. I'll add tests for the validation function, as well as a functional test for raising the error when attempting a write. URI parsing will get a consistent error reporting and I'll add the failing case for "mongodb://host/?w=0&j=true". I'll follow up once the PR is updated. |
| Comment by Jeremy Mikola [ 19/Mar/15 ] |
|
On second thought, BSON_ASSERT() was a premature suggestion. I'll follow up in a bit with a more robust solution. |
| Comment by A. Jesse Jiryu Davis [ 19/Mar/15 ] |
|
Sounds great Jeremy thanks! Let's include a test of the URI too? "mongodb://host/?w=0&j=true" shouldn't parse. |
| Comment by Jeremy Mikola [ 19/Mar/15 ] |
|
I concur that w=0 combined with either j=true or fsync=true is a logical error; however, the C driver's write concern component doesn't have a convention for reporting errors if the user decides to set the struct to such a state. I'm happy to add some error reporting to the PR to address that, if we can agree on how to go about it. At a glance, I expect BSON_ASSERT() calls on the setter methods in mongoc-write-concern.c would suffice (instead of catching all of the places that use a write concern and call _mongoc_write_concern_needs_gle(). I believe that would also implicitly handle concerns with expressing an invalid write concern during URI parsing. |
| Comment by A. Jesse Jiryu Davis [ 19/Mar/15 ] |
|
bjori I completely agree with you that only providing j=true implies "acknowledged". In this case no "w" value is sent to the server, we accept the server's default "w" value. But it looks like we should treat w=0, j=true as an error in the C Driver, not as an acknowledged write. |
| Comment by David Golden [ 19/Mar/15 ] |
|
FWIW, the next gen perl driver also considers {w:0, j:1}to be an error: https://github.com/mongodb/mongo-perl-driver/blob/master/lib/MongoDB/WriteConcern.pm#L82-L84 |
| Comment by Bernie Hackett [ 19/Mar/15 ] |
|
Passing j=true with legacy op codes requires we send a getLastError command to the server, and hence get an acknowledgement. There is no way around it. Passing w=0 means "don't ask for acknowledgement". I think passing w=0&j=true should be an error. There is no way to do what the user is asking for. Also, I wasn't aware that passing {w: 0, j: true}to a write command forced the server to upgrade the w value to 1. Though that's interesting by itself, it doesn't change the fact that the user is requesting something that is not possible. Not to mention the fact that drivers are supposed to use legacy op codes with w=0, not write commands, since write commands always return a response. |
| Comment by Hannes Magnusson [ 19/Mar/15 ] |
|
Using the older Mongo objects (as apposed to MongoClient) and only providing j=true in the connection uri will result in the reported behaviour. If setting w=0&j=true on MongoClient is deemed an error, the ticket is still valid as there is no such error issued and the write concern is silently downgraded. |
| Comment by Bernie Hackett [ 19/Mar/15 ] |
|
In PyMongo 3.0 we've made this really clear by raising an error: https://github.com/mongodb/mongo-python-driver/blob/master/pymongo/write_concern.py#L72-L74 |
| Comment by Bernie Hackett [ 19/Mar/15 ] |
This seems amazingly counter-intuitive. Setting w=0 means "don't acknowledge my writes". The Java and python drivers do the opposite of what you propose, and always have. |
| Comment by A. Jesse Jiryu Davis [ 19/Mar/15 ] |
|
I believe the desired behavior, making "w: 0, j: true" an acknowledged op, diverges from other drivers. PyMongo considers "w: 0" to be the user's express intention to do unacknowledged legacy writes. There's no spec of course. behackett, jeff.yemin, care to comment? Of course if the user simply specifies "j: true", the driver should call getlasterror with "j: true" and no "w" at all (the default, acknowledged write concern). Same for the "writeConcern" document sent with write commands. |
| Comment by Jeremy Mikola [ 13/Mar/15 ] |