Optimize the case when `_version` is never specified / always 0

[mikemccand]

@rmuir had this clever idea: because of how we compress things, Lucene's codec knows when a given doc values field is always constant, and we could use this to know that _version is always 0, and do sizable optimizations like skip the per-doc _uid lookup during indexing, don't record version in the live version map (though we still use it for e.g. real-time get, see #19787 and #19813).

This could be a nice solution because it wouldn't require any configuration on the user's part to state that they will not use optimistic concurrency features: the engine would just notice and be more efficient in this very common case. And if users do use optimistic concurrency features, it would be slower.

But one problem here is the engine automatically increments the version each time a _uid is re-indexed: can we stop doing this auto-increment?

[rmuir]

To do this cleaner (assuming semantics are fixed), we should leave version empty instead of putting 0's in there. That is done for some crazy reason involving nested docs today, and should be cleaned up.

Then things are obvious: if it is not in use, it does not exist in fieldinfos at all. truly empty instead of 0.

[jimczi]

But one problem here is the engine automatically increments the version each time a _uid is re-indexed: can we stop doing this auto-increment?

Why is it a problem ? For append only indices this should never happen ? The auto-increment is an optimization since we don't check if the document has changed or not when we increment the version.

[rmuir]

It is a problem because it requires an expensive PK lookup to see if the document already exists.

For the append only use case, this is a complete waste of time.

[jimczi]

It is a problem because it requires an expensive PK lookup to see if the document already exists.

The fact that we check if the document already exists is another problem, no ? I might be missing something but I don't see why we could bypass this check if the _version field is constant among the segments. The fact that all documents in the index are in their initial version does not ensure that there will be no updates in the future.

[rmuir]

There are two PK checks:

1. the PK lookup + version check done by elasticsearch
2. the pk lookup check done by indexwriter (since updateDocument was called)

Both are unnecessary in an append-only case. This is only about the first one though. If the semantics are we drop updates to a document with version X when, the document already exists with version Y and Y > X, then we don't need to do a bunch of individual, expensive, per-document lookups if there is not a single document in the entire index with any version > X.

[jimczi]

Ah right, two PK checks. Thanks for the explanation.

[rmuir]

Yes the first one i think is the most costly, real random access, also requires tracking uncommitted PKs with concurrent hash maps, and so on.

The second one is batched by indexwriter and then a bunch of ids done in big sequential passes, doesn't require the use of additional memory (version maps): i think its the lesser evil, we should worry about it separately.

[jpountz]

For the record, Elasticsearch calls addDocument rather than updateDocument when the version lookup returns NOT_FOUND so in the append-only case there would be a single PK lookup per document (but the costly one). I believe we could replace it with updateDocument by using a version number that is guaranteed to be greater than the current version of the document (eg. by allocating versions using an AtomicLong) but this would mean the _version would use disk space and we would not be able to know whether the document was created or updated (which is something we return in the response today unfortunately).

[rmuir]

I don't understand why we need to store a version at all. Whenever I inspect elasticsearch indexes today, none of the _version fields have any actual values!

[rmuir]

and we would not be able to know whether the document was created or updated (which is something we return in the response today unfortunately).

it is frustrating to read this when making these discussions. It comes across as "its always been this way, therefore cannot be changed". I don't think its intentional, it just comes across as "your idea cannot work because we return X in the response, because we need to check Y consistency thing, because of Z", when I am trying to say, all of X, Y, Z, I think its all broken.

When I look at the _version field of any elasticsearch index i ever see, it is always unused (zeros). So I know its possible for it to function without these stuff, since its literally storing a whole whopping amount of nothing in all of these cases. I dont want to hear "we would need disk space" because I know that is not needed, its not happening today and things work!

In other words, I don't think we need to worry about X, Y, or Z at all, lets change that stuff. I'm making the statement that the current semantics are inefficient. Its frustrating to see the indexing speeds when lucene is a lot more efficient than this and its only a matter of time before someone else catches on. I think the semantics should be changed.

[jpountz]

It comes across as "its always been this way, therefore cannot be changed"

Sorry, this is not what I meant. On the contrary I'm all for removing all these features that are in the way of improving indexing speed: let's remove configurable version types, information about whether the document was created of updated in the response, etc. I mentioned the latter since it is about the response format of a commonly used API, which means we need to be careful, but this is no excuse to not do the change. The fact that it is in the way of indexing speed improvements means that we will want to fix it at some point, so the sooner, the better.

[nik9000]

I mentioned the latter since it is about the response format of a commonly used API, which means we need to be careful, but this is no excuse to not do the change

We can break the format in 6.0 if we want. We'll branch for that before long.

If we're doing the append only case we could probably just return _version as 0 in the response anyway, right? So maybe we don't have to change the API if we don't want to?