ESQL: Fix usage of already released null block in ValueSourceReaderOperator #126411

alex-spies · 2025-04-07T14:50:32Z

elasticsearchmachine · 2025-04-07T14:50:57Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2025-04-07T14:50:57Z

Hi @alex-spies, I've created a changelog YAML for you.

alex-spies · 2025-04-07T14:53:39Z

In addition to the enclosed IT, I'd love to have a unit test for this.

It looks like we should be able to add a case to ValueSourceReaderOperatorTests, but I struggled to build one that travels the specific code path with the bug, though. The reason is that the bug only happens when we re-use the same ComputeBlockLoaderFactory and create null blocks for multiple pages; but this only happens when the doc block doesn't belong to a single shard, but multiple ones.

@nik9000 if you have an idea how to add a test without major effort, I'd love to add it, too.

elasticsearchmachine · 2025-04-07T15:13:36Z

Hi @alex-spies, I've updated the changelog YAML for you.

.../esql/compute/src/main/java/org/elasticsearch/compute/lucene/ValuesSourceReaderOperator.java

nik9000 · 2025-04-07T19:21:11Z

.../esql/compute/src/main/java/org/elasticsearch/compute/lucene/ValuesSourceReaderOperator.java

+            // However, downstream operators take ownership of the null block we hand to them and may close it, forcing us to create a new
+            // one.
+            // This could be avoided altogether if this factory itself kept a reference to the null block, but we'd have to also ensure
+            // to release this block once the factory is not used anymore.


Right, the factory itself would have to be Releasable. FWIW, that's not really a big deal. I think I'd prefer it to this fix to be honest. I'm kind of ok with whichever though.

But if you are trying to get this merged quickly I'd be fine with this solution - especially if you are willing to try the other in a followup.

I also prefer making this Releasable. It's easy to do, I'll push a commit with the change.

Wrong implementations of .close() or forgetting to incref when handing out blocks actually nicely show up as test failures already in the existing tests, so I think this is guarding better against regressions.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

…y-released-null-block

alex-spies · 2025-04-08T09:37:26Z

.../esql/compute/src/main/java/org/elasticsearch/compute/lucene/ValuesSourceReaderOperator.java

+                int p = forwards[0];
+                int shard = shards.getInt(p);
+                int segment = segments.getInt(p);
+                int firstDoc = docs.getInt(p);
+                positionFieldWork(shard, segment, firstDoc);
+                LeafReaderContext ctx = ctx(shard, segment);
+                fieldsMoved(ctx, shard);
                verifyBuilders(loaderBlockFactory, shard);
-                read(docs.getInt(p), shard);
+                read(firstDoc, shard);
+                for (int i = 1; i < forwards.length; i++) {
+                    p = forwards[i];
+                    shard = shards.getInt(p);
+                    segment = segments.getInt(p);
+                    boolean changedSegment = positionFieldWorkDocGuarteedAscending(shard, segment);
+                    if (changedSegment) {
+                        ctx = ctx(shard, segment);
+                        fieldsMoved(ctx, shard);
+                    }
+                    verifyBuilders(loaderBlockFactory, shard);
+                    read(docs.getInt(p), shard);
+                }


This just got indented, the diff is actually just whitespace o.O

If you add ?w=1 to the url it'll ignore whitespace ala git diff -w. There's an option for it hiding behind the gear icon at the top of the page too.

nik9000

I like it much more now. Thanks!

alex-spies · 2025-04-08T15:15:48Z

The Serverless checks are failing with an unrelated test failure that is already known (Serverless CI issue opened last week, re-muted 4h ago).

I think it's not worth updating the branch to run CI again just to pick up on the mute for this test, so I'm going to go ahead with the merge as all other tests and CI checks passed.

elasticsearchmachine · 2025-04-08T15:19:57Z

💔 Backport failed

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

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

…erator (elastic#126411) * Add yaml test with reproducer * Fix the bug * Make ComputeBlockLoaderFactory Releasable

alex-spies · 2025-04-08T15:35:27Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

…erator (elastic#126411) * Add yaml test with reproducer * Fix the bug * Make ComputeBlockLoaderFactory Releasable (cherry picked from commit ffd4913) # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

…erator (#126411) (#126475) * Add yaml test with reproducer * Fix the bug * Make ComputeBlockLoaderFactory Releasable

…erator (#126411) (#126474) * Add yaml test with reproducer * Fix the bug * Make ComputeBlockLoaderFactory Releasable

…erator (#126411) (#126476) * Add yaml test with reproducer * Fix the bug * Make ComputeBlockLoaderFactory Releasable

…erator (#126411) (#126480) * Add yaml test with reproducer * Fix the bug * Make ComputeBlockLoaderFactory Releasable (cherry picked from commit ffd4913) # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

alex-spies added 2 commits April 7, 2025 13:53

Add yaml test with reproducer

ac04e86

Fix the bug

d8d94a9

alex-spies added >bug auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.18.1 v8.19.0 v9.0.1 v9.1.0 v8.17.5 labels Apr 7, 2025

alex-spies requested review from craigtaverner and nik9000 April 7, 2025 14:50

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 7, 2025

Update docs/changelog/126411.yaml

e9784e1

idegtiarenko approved these changes Apr 7, 2025

View reviewed changes

Update docs/changelog/126411.yaml

040fd39

nik9000 reviewed Apr 7, 2025

View reviewed changes

alex-spies added 3 commits April 8, 2025 11:35

Make ComputeBlockLoaderFactory Releasable

78e47b4

Update capability description

3de2dcd

Merge remote-tracking branch 'upstream/main' into fix-usage-of-alread…

f443a82

…y-released-null-block

alex-spies commented Apr 8, 2025

View reviewed changes

alex-spies requested a review from nik9000 April 8, 2025 09:38

nik9000 approved these changes Apr 8, 2025

View reviewed changes

alex-spies added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Apr 8, 2025

alex-spies merged commit ffd4913 into elastic:main Apr 8, 2025
16 of 17 checks passed

alex-spies deleted the fix-usage-of-already-released-null-block branch April 8, 2025 15:18

elasticsearchmachine added the backport pending label Apr 8, 2025

alex-spies mentioned this pull request Apr 8, 2025

[8.x] ESQL: Fix usage of already released null block in ValueSourceReaderOperator (#126411) #126480

Merged

elasticsearchmachine pushed a commit that referenced this pull request Apr 8, 2025

ESQL: Fix usage of already released null block in ValueSourceReaderOp…

48c2008

…erator (#126411) (#126475) * Add yaml test with reproducer * Fix the bug * Make ComputeBlockLoaderFactory Releasable

elasticsearchmachine pushed a commit that referenced this pull request Apr 8, 2025

ESQL: Fix usage of already released null block in ValueSourceReaderOp…

c99983e

…erator (#126411) (#126474) * Add yaml test with reproducer * Fix the bug * Make ComputeBlockLoaderFactory Releasable

elasticsearchmachine pushed a commit that referenced this pull request Apr 8, 2025

ESQL: Fix usage of already released null block in ValueSourceReaderOp…

634048d

…erator (#126411) (#126476) * Add yaml test with reproducer * Fix the bug * Make ComputeBlockLoaderFactory Releasable

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Fix usage of already released null block in ValueSourceReaderOperator #126411

ESQL: Fix usage of already released null block in ValueSourceReaderOperator #126411

alex-spies commented Apr 7, 2025 •

edited

Loading

elasticsearchmachine commented Apr 7, 2025

elasticsearchmachine commented Apr 7, 2025

alex-spies commented Apr 7, 2025

elasticsearchmachine commented Apr 7, 2025

nik9000 Apr 7, 2025

nik9000 Apr 7, 2025

alex-spies Apr 8, 2025

alex-spies Apr 8, 2025

nik9000 Apr 8, 2025

nik9000 left a comment

alex-spies commented Apr 8, 2025

elasticsearchmachine commented Apr 8, 2025

alex-spies commented Apr 8, 2025

ESQL: Fix usage of already released null block in ValueSourceReaderOperator #126411

ESQL: Fix usage of already released null block in ValueSourceReaderOperator #126411

Conversation

alex-spies commented Apr 7, 2025 • edited Loading

elasticsearchmachine commented Apr 7, 2025

elasticsearchmachine commented Apr 7, 2025

alex-spies commented Apr 7, 2025

elasticsearchmachine commented Apr 7, 2025

nik9000 Apr 7, 2025

Choose a reason for hiding this comment

nik9000 Apr 7, 2025

Choose a reason for hiding this comment

alex-spies Apr 8, 2025

Choose a reason for hiding this comment

alex-spies Apr 8, 2025

Choose a reason for hiding this comment

nik9000 Apr 8, 2025

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

alex-spies commented Apr 8, 2025

elasticsearchmachine commented Apr 8, 2025

💔 Backport failed

alex-spies commented Apr 8, 2025

💚 All backports created successfully

Questions ?

alex-spies commented Apr 7, 2025 •

edited

Loading