Use new source loader when lower docId is accessed #128320

parkertimmins · 2025-05-22T15:25:09Z

When using synthetic source, runtime fields data may come from doc values. Doc values iterators can only be read once, and in increasing docId order. But if a runtime field is referenced multiple times in a query, currently the same doc value iterator will be used. This causes an error, as the second field reference will attempt to read the same iterator from a lower docId than was previously used. The fix is to create a new source loader, and thus a new doc value iterator, if the requested docId is lower than the last seen docId.

elasticsearchmachine · 2025-05-22T15:25:33Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

elasticsearchmachine · 2025-05-22T15:25:34Z

Hi @parkertimmins, I've created a changelog YAML for you.

parkertimmins · 2025-05-22T15:28:26Z

@jimczi Added you as reviewer on this, since if modifies code that you recently added

martijnvg

Left two comments, LGTM otherwise.

I think we should also backport to the 8.18 branch?

martijnvg · 2025-05-23T05:52:19Z

x-pack/plugin/logsdb/src/javaRestTest/java/org/elasticsearch/xpack/logsdb/LogsdbRestIT.java

+        String indexName = "test-foo";
+        createIndex(indexName, Settings.builder().put("index.mode", "logsdb").build(), mappings);
+
+        int numDocs = 100_000;


The amount of data that this test generates / indexes, makes this is an expensive test. Does the problem re-occur if we lower the number of documents here?

Ahh yep, 100k is a lot. Your original test example actually triggers the issue with 10 docs. Reverting the test to be closer to your original version, and decreasing to 10 docs.

martijnvg · 2025-05-23T05:53:29Z

x-pack/plugin/logsdb/src/javaRestTest/java/org/elasticsearch/xpack/logsdb/LogsdbRestIT.java

+        assertThat(shardsHeader.get("failed"), equalTo(0));
+        assertThat(shardsHeader.get("successful"), equalTo(1));
+        assertThat(shardsHeader.get("skipped"), equalTo(0));
+        logger.info("searchResponse: {}", searchResponseBody);


Maybe also assert total hits here?

martijnvg · 2025-05-23T05:57:42Z

server/src/main/java/org/elasticsearch/search/lookup/ConcurrentSegmentSourceProvider.java

@@ -44,6 +46,11 @@ public Source getSource(LeafReaderContext ctx, int doc) throws IOException {
            leaf = new Leaf(sourceLoader.leaf(ctx.reader(), null), storedFieldLoader.getLoader(ctx, null));
            var existing = leaves.put(id, leaf);
            assert existing == null : "unexpected source provider [" + existing + "]";
+        } else if (isStoredSource == false && doc < leaf.doc) {
+            // For synthetic source, if a runtime field is used more than once, a new source loader must be used


Maybe also incorporate the following into this comment: with both synthetic and stored source the docid can go backwards. This isn't a problem for stored fields (somehow it validate when docid goes backwards and doesn't run into EOF like errors), but it is for doc values (which gets used with synthetic source). ?

The issue here seems to be when multiple clauses in a query reference runtime fields on the same request, no? So it's not only about using the same runtime field twice.

The issue here seems to be when multiple clauses in a query reference runtime fields on the same request

Yes I think that is a more accurate way to describe the issue. I confirmed that, unsurprisingly, aggregating twice on the same runtime field does indeed work fine as is. So this is specific to there being 2+ query clauses referencing the same runtime field.

jimczi

Good catch!

jimczi · 2025-05-23T07:24:44Z

server/src/main/java/org/elasticsearch/search/lookup/ConcurrentSegmentSourceProvider.java

@@ -44,6 +46,11 @@ public Source getSource(LeafReaderContext ctx, int doc) throws IOException {
            leaf = new Leaf(sourceLoader.leaf(ctx.reader(), null), storedFieldLoader.getLoader(ctx, null));
            var existing = leaves.put(id, leaf);
            assert existing == null : "unexpected source provider [" + existing + "]";
+        } else if (isStoredSource == false && doc < leaf.doc) {
+            // For synthetic source, if a runtime field is used more than once, a new source loader must be used


The issue here seems to be when multiple clauses in a query reference runtime fields on the same request, no? So it's not only about using the same runtime field twice.

elasticsearchmachine · 2025-05-23T20:59:04Z

💔 Backport failed

Status	Branch	Result
✅	8.19
❌	9.0	Commit could not be cherrypicked due to conflicts
❌	8.17	Commit could not be cherrypicked due to conflicts
❌	8.18	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 128320

When using synthetic source, runtime fields data may come from doc values. Doc values iterators can only be read once, and in increasing docId order. But if a runtime field is referenced multiple times in a query, currently the same doc value iterator will be used. This causes an error, as the second field reference will attempt to read the same iterator from a lower docId than was previously used. The fix is to create a new source loader, and thus a new doc value iterator, if the requested docId is lower than the last seen docId.

When using synthetic source, runtime fields data may come from doc values. Doc values iterators can only be read once, and in increasing docId order. But if a runtime field is referenced multiple times in a query, currently the same doc value iterator will be used. This causes an error, as the second field reference will attempt to read the same iterator from a lower docId than was previously used. The fix is to create a new source loader, and thus a new doc value iterator, if the requested docId is lower than the last seen docId. (cherry picked from commit 51e87cb) # Conflicts: # server/src/main/java/org/elasticsearch/search/lookup/ConcurrentSegmentSourceProvider.java

parkertimmins · 2025-05-23T21:32:51Z

💚 All backports created successfully

Status	Branch	Result
✅	9.0
✅	8.18
✅	8.17

Questions ?

Please refer to the Backport tool documentation

When using synthetic source, runtime fields data may come from doc values. Doc values iterators can only be read once, and in increasing docId order. But if a runtime field is referenced multiple times in a query, currently the same doc value iterator will be used. This causes an error, as the second field reference will attempt to read the same iterator from a lower docId than was previously used. The fix is to create a new source loader, and thus a new doc value iterator, if the requested docId is lower than the last seen docId.

When using synthetic source, runtime fields data may come from doc values. Doc values iterators can only be read once, and in increasing docId order. But if a runtime field is referenced multiple times in a query, currently the same doc value iterator will be used. This causes an error, as the second field reference will attempt to read the same iterator from a lower docId than was previously used. The fix is to create a new source loader, and thus a new doc value iterator, if the requested docId is lower than the last seen docId. (cherry picked from commit 51e87cb) # Conflicts: # server/src/main/java/org/elasticsearch/search/lookup/ConcurrentSegmentSourceProvider.java

When using synthetic source, runtime fields data may come from doc values. Doc values iterators can only be read once, and in increasing docId order. But if a runtime field is referenced multiple times in a query, currently the same doc value iterator will be used. This causes an error, as the second field reference will attempt to read the same iterator from a lower docId than was previously used. The fix is to create a new source loader, and thus a new doc value iterator, if the requested docId is lower than the last seen docId.

parkertimmins added >bug auto-backport Automatically create backport pull requests when merged :StorageEngine/Codec v8.19.0 v9.1.0 labels May 22, 2025

elasticsearchmachine added the Team:StorageEngine label May 22, 2025

Update docs/changelog/128320.yaml

8d27a83

parkertimmins requested review from martijnvg and jimczi and removed request for martijnvg May 22, 2025 15:27

martijnvg approved these changes May 23, 2025

View reviewed changes

jimczi approved these changes May 23, 2025

View reviewed changes

parkertimmins and others added 5 commits May 23, 2025 10:04

Previous version of test fails with less docs

119fee3

simplify test to minimal necessary to trigger behavior

dd8a0c8

update comment

2609ed2

[CI] Auto commit changes from spotless

25bc848

shorten comment

d4e7062

parkertimmins added v8.18.3 v9.0.3 v8.17.8 labels May 23, 2025

parkertimmins added 2 commits May 23, 2025 14:34

Merge branch 'main' into parker/synthetic-source-runtime-doc-value-reset

267dc73

Increase numDocs to make sure triggers behavior

0f1b160

parkertimmins merged commit 51e87cb into elastic:main May 23, 2025
18 checks passed

parkertimmins deleted the parker/synthetic-source-runtime-doc-value-reset branch May 23, 2025 20:57

parkertimmins mentioned this pull request May 23, 2025

[8.19] Use new source loader when lower docId is accessed (#128320) #128408

Merged

elasticsearchmachine added the backport pending label May 23, 2025

parkertimmins mentioned this pull request May 23, 2025

[9.0] Use new source loader when lower docId is accessed (#128320) #128409

Merged

parkertimmins mentioned this pull request May 23, 2025

[8.18] Use new source loader when lower docId is accessed (#128320) #128410

Merged

parkertimmins mentioned this pull request May 23, 2025

[8.17] Use new source loader when lower docId is accessed (#128320) #128411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use new source loader when lower docId is accessed #128320

Use new source loader when lower docId is accessed #128320

Uh oh!

parkertimmins commented May 22, 2025

Uh oh!

elasticsearchmachine commented May 22, 2025

Uh oh!

elasticsearchmachine commented May 22, 2025

Uh oh!

parkertimmins commented May 22, 2025

Uh oh!

martijnvg left a comment

Uh oh!

martijnvg May 23, 2025

Uh oh!

parkertimmins May 23, 2025

Uh oh!

martijnvg May 23, 2025

Uh oh!

martijnvg May 23, 2025

Uh oh!

jimczi May 23, 2025

Uh oh!

parkertimmins May 23, 2025

Uh oh!

jimczi left a comment

Uh oh!

jimczi May 23, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented May 23, 2025

Uh oh!

parkertimmins commented May 23, 2025

Uh oh!

Uh oh!

Use new source loader when lower docId is accessed #128320

Use new source loader when lower docId is accessed #128320

Uh oh!

Conversation

parkertimmins commented May 22, 2025

Uh oh!

elasticsearchmachine commented May 22, 2025

Uh oh!

elasticsearchmachine commented May 22, 2025

Uh oh!

parkertimmins commented May 22, 2025

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

martijnvg May 23, 2025

Choose a reason for hiding this comment

Uh oh!

parkertimmins May 23, 2025

Choose a reason for hiding this comment

Uh oh!

martijnvg May 23, 2025

Choose a reason for hiding this comment

Uh oh!

martijnvg May 23, 2025

Choose a reason for hiding this comment

Uh oh!

jimczi May 23, 2025

Choose a reason for hiding this comment

Uh oh!

parkertimmins May 23, 2025

Choose a reason for hiding this comment

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

jimczi May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented May 23, 2025

💔 Backport failed

Uh oh!

parkertimmins commented May 23, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Uh oh!