[SERVER-19924] getLastError's n value now indicates the number of inserts completed. Created: 13/Aug/15  Updated: 20/Aug/15  Resolved: 20/Aug/15

Status: Closed
Project: Core Server
Component/s: Write Ops
Affects Version/s: 3.1.7
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Craig Wilson Assignee: David Storch
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
is related to SERVER-18131 Make LastError data part of Client, n... Closed
Operating System: ALL
Steps To Reproduce:

In 3.0:

> db.foo.drop();
> db.runCommand({insert: "foo", documents: [ { _id: 1} ]})
{ "ok" : 1, "n" : 1 }
> db.runCommand({getLastError: 1})
{
        "connectionId" : 1,
        "n" : 0,
        "syncMillis" : 0,
        "writtenTo" : null,
        "err" : null,
        "ok" : 1
}

In 3.1:

> db.foo.drop();
> db.runCommand({insert: "foo", documents: [ { _id: 1} ]})
{ "ok" : 1, "n" : 1 }
> db.runCommand({getLastError: 1})
{
        "connectionId" : 1,
        "n" : 1,
        "syncMillis" : 0,
        "writtenTo" : null,
        "err" : null,
        "ok" : 1
}

Participants:

 Description   

getLastError used to return 0 for "n" after an insert. With the latest 3.1.7 nightly, it is now returning 1 (or presumably, the number of documents inserted).

I certainly think this is the right value for "n" if we're starting from scratch, but at this point we shouldn't change this behavior as users may be relying on this.



 Comments   
Comment by Craig Wilson [ 20/Aug/15 ]

Thanks David. I don't think either one holds true. All my tests of current/older .NET driver versions all pass except for where I'm asserting the n value. I think this will probably hold true for other drivers as well. So, let's mark the change as Drivers Changes Needed and leave it in.

Comment by David Storch [ 19/Aug/15 ]

craiggwilson, got it. I am inclined to close this as Works as Designed with Drivers Changes Needed. In general I think that we should fix bugs on the server, even if they amount to a behavior change in a publicly documented API. The exception to this would be if it any of the following are true:

  1. The change puts an unnecessary burden on driver authors for maintaining forwards/backwards compatibility.
  2. The change is backwards breaking from the perspective of a user application.

Do either of these hold true? Of course the change will be backwards breaking if a user calls getLastError directly, but it sounds like it probably won't change from the application's perspective for getLastError invoked by the driver? Are the required drivers changes going to be difficult?

Comment by Craig Wilson [ 19/Aug/15 ]

All I know is that this is a change in behavior to a publicly documented API. I have no idea who might be relying on this, only that the output will be different.

1. It only applies to getLastError, which is all but deprecated already due to the introduction of the write commands. Why change something that we are effectively not using?
2. All drivers have already handled this "bug" in order to report the "n" value correctly to the affect that making a change will have no beneficial impact from a driver perspective. However, I'm am unsure if making this change would cause undue hardship on a driver while interpreting results. At the very least, drivers should do a server version check (in whatever capacity that looks like) to determine whether to make up an "n" value or to use the one from the server. For instance, now that "n" for inserts does not always equal 0, 0 now has substantial meaning in server >= 3.2.

All in all, like I said in the description, I think this is the correct value for n. If you decide you want it fixed, then make sure to mark this as Drivers Changes Needed so all drivers can evaluate their codebases and make the appropriate changes.

Comment by David Storch [ 19/Aug/15 ]

craiggwilson, my reading of the commit that introduced this behavior change is that this was an explicit bug fix packaged with some other getLastError refactoring. In particular, the old implementation recorded the number of inserts here:

https://github.com/mongodb/mongo/blob/bdbe140e3cc502a58d50a8ec472bd9c05cf12e4f/src/mongo/db/commands/write_commands/batch_executor.cpp#L553

This sets nObjects to the number of documents inserted, but fails to mark the LastError instance as "valid". The downstream effect is that the number of inserts are misreported in the getLastError command response.

What's your argument for why we should revert this bug fix? Who might be depending on this behavior in a way that would break after upgrade to 3.2?

Comment by David Storch [ 19/Aug/15 ]

This behavior change was introduced by commit da31be34dc66 under SERVER-18131.

CC schwerin.

Generated at Thu Feb 08 03:52:35 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.