[BUG] SQL legacy engine creates PIT unnecessarily and leak context

[dai-chen]

What is the bug?

When no cursor requested (fetch_size is not specified), the SQL Legacy engine creates Point-in-Time (PIT) contexts unnecessarily. Meanwhile, the PIT is also not closed properly, causing PIT contexts to leak and accumulate until they expire after the keep-alive timeout (default: 1 minute). Under repeated or concurrent query workloads, this can exhaust PIT capacity and hit the default limit of 300 open PIT contexts, resulting in query failures.

How can one reproduce the bug?

1. Create test index:

PUT /test-logs-2025-01-01
{
  "mappings": {
    "properties": {
      "action": {"type": "text", "fields": {"keyword": {"type": "keyword"}}},
      "timestamp": {"type": "date"}
    }
  }
}

2. Insert test data:

POST /test-logs-2025-01-01/_bulk
{"index":{}}
{"action":"login_success","timestamp":"2025-01-01T10:00:00Z"}
{"index":{}}
{"action":"login_success","timestamp":"2025-01-01T10:01:00Z"}
{"index":{}}
{"action":"login_failed","timestamp":"2025-01-01T10:02:00Z"}

3. Run SQL query (with backtick for V2 test, e.g., `test-logs-2025-01-01` ):

POST /_plugins/_sql
{
  "query": "SELECT * FROM test-logs-2025-01-01 WHERE action LIKE 'login%' ORDER BY timestamp ASC"
}

4. Check PIT contexts:

GET /_nodes/stats/indices/search?filter_path=nodes.*.indices.search.point_in_time_current

What is the expected behavior?

For queries that complete in a single page (no cursor requested / no follow-up page needed):

• Do not create PIT, or;
• If PIT is created internally, close/delete it before returning the response.

Note: The SQL/PPL V2 engine may also create PIT implicitly, but in that case it can be intentional: V2 may fetch until the last page and then close the PIT to ensure correctness for any required in-memory post-processing.

What is your host/environment?

• OpenSearch Version: 2.19-dev, main branch
• Plugin: SQL plugin

Do you have any screenshots?

N/A

Do you have any additional context?

• Legacy engine (SQL): always creates PIT aggressively when handling queries. The conditional check from 2.5 was removed.
  • Code: https://github.com/opensearch-project/sql/blob/main/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java#L103
• V2 Engine (SQL/PPL): PIT is created when from + size > maxResultWindow. Request size is Integer.MAX_VALUE by default and size limit setting is not applied. The V2 engine closes PIT contexts by fetching all pages until an empty response is received, then deleting the PIT to ensure no PIT leak.
  • 2.5 code: https://github.com/opensearch-project/sql/blob/2.5/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java#L99
  • main code: https://github.com/opensearch-project/sql/blob/main/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java#L63
• V3 Engine (Calcite - PPL only): doesn't have the issue because Calcite optimizer automatically applies a system size limit to every query. The system limit is read from a setting which is 10k by default.
  • Doc: https://github.com/opensearch-project/sql/blob/main/docs/user/admin/settings.rst#plugins-query-size-limit
  • Code: https://github.com/opensearch-project/sql/blob/main/core/src/main/java/org/opensearch/sql/executor/QueryService.java#L284

[coderabbitai]

📝 CodeRabbit Plan Mode

Generate an implementation plan and prompts that you can use with your favorite coding agent.

✅ Successfully generated implementation plan.

💡 Use @coderabbitai <feedback> to replan

Examples

• Example 1
• Example 2

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[coderabbitai]

Coding Plan

Approach

Ticket Summary

Fix the SQL Legacy engine to prevent unnecessary Point-in-Time (PIT) context creation when no cursor is requested (fetch_size not specified) and ensure proper PIT cleanup to prevent resource leaks that can exhaust the default limit of 300 open PITs under load.

Observations

Codebase Summary

The OpenSearch SQL plugin Legacy engine executes queries through PrettyFormatRestExecutor, which delegates to DefaultQueryAction for query building and uses PointInTimeHandlerImpl for PIT management. The current implementation in PrettyFormatRestExecutor.buildProtocolForDefaultQuery() unconditionally creates a PIT for all DefaultQueryAction queries but never cleans it up. The codebase already has the necessary decision logic (checkIfScrollNeeded() in DefaultQueryAction) and cleanup patterns (try-finally blocks with explicit pit.delete() calls used in other executors like ElasticJoinExecutor and MinusExecutor). The fetch_size parameter is validated and stored in SqlRequest, accessible via queryAction.getSqlRequest().fetchSize(), and determines whether cursor-based pagination is needed.

Assumptions

Assumption 1: When should PIT be created for Legacy engine queries?

Options Considered:

• Always create PIT and add cleanup logic (safe but wasteful)
• Create PIT conditionally based on fetch_size presence (aligns with ticket requirements)
• Match V2's maxResultWindow pattern exactly (requires significant refactoring)

Chosen Option: Create PIT conditionally when fetch_size > 0, which indicates the user requested cursor-based pagination

Rationale: This aligns with the ticket's "do not create PIT" requirement for single-page queries, leverages existing fetch_size validation logic, and matches the developer's intent when specifying fetch_size. The codebase already has checkIfScrollNeeded() that uses this same condition, confirming this is the established pattern for determining pagination necessity.

Assumption 2: How should PIT cleanup be implemented?

Options Considered:

• Try-with-resources pattern (requires PointInTimeHandler to implement AutoCloseable)
• Try-finally with explicit delete() calls (matches existing patterns)
• Cleanup only in success path (risky, may leak on errors)

Chosen Option: Use try-finally blocks with explicit pit.delete() calls and error handling

Rationale: Following the established pattern in ElasticJoinExecutor and MinusExecutor. This approach is already proven in the Legacy codebase, handles edge cases where PIT is created but cursor isn't generated, and includes proper error logging with metrics tracking that developers expect in production code.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Add Conditional PIT Creation Logic

Modify PrettyFormatRestExecutor to check if cursor-based pagination is requested before creating PIT, preventing unnecessary PIT creation for single-page queries.

Task 1: Add fetch_size Check Before PIT Creation

Update the buildProtocolForDefaultQuery() method in legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java to conditionally create PIT only when fetch_size > 0.

• Check queryAction.getSqlRequest().fetchSize() before creating PointInTimeHandlerImpl
• Only instantiate PointInTimeHandlerImpl and call pit.create() when fetchSize is greater than 0
• Keep existing PIT setup logic (setPointInTime on SearchRequestBuilder) within the conditional block
• Ensure PointInTimeHandler variable is declared outside the conditional block (initialized to null) so it's accessible for cleanup logic

Task 2: Handle Non-PIT Query Path

Ensure search execution works correctly when PIT is not created.

• When fetch_size is 0 or not specified, execute search request without PIT setup
• The SearchRequestBuilder should proceed with standard search execution (no setPointInTime call)
• Maintain existing response handling and Protocol formatting logic unchanged

🤖 Prompt for AI agents

Modify PrettyFormatRestExecutor.buildProtocolForDefaultQuery() to conditionally
create PIT only when cursor-based pagination is needed.

In buildProtocolForDefaultQuery():
- Declare PointInTimeHandler variable initialized to null
- Check if queryAction.getSqlRequest().fetchSize() > 0
- Only create PointInTimeHandlerImpl and call pit.create() when fetchSize > 0
- Only call setPointInTime on SearchRequestBuilder when PIT is created
- Execute search request normally whether PIT is created or not

Phase 2: Add PIT Cleanup for Edge Cases

Implement proper PIT lifecycle management with try-finally blocks to ensure PITs are deleted when not used for cursor-based pagination, following the cleanup pattern established in other Legacy executors.

Task 1: Add Try-Finally Block Around Search Execution

Wrap the search execution and cursor creation logic in a try-finally block in PrettyFormatRestExecutor.buildProtocolForDefaultQuery().

• Move search request execution (searchRequest.get()) into try block
• Move cursor creation check (isDefaultCursor()) and cursor building logic into try block
• Add finally block after cursor creation logic to handle PIT cleanup
• Ensure Protocol object creation and return statement remain outside the try-finally (after cleanup)

Task 2: Implement Conditional PIT Deletion

Add cleanup logic in the finally block to delete PIT when it was created but no cursor was generated.

• In finally block, check if pit != null (meaning PIT was created)
• Check if cursor was NOT created (need to track this with a boolean flag set in the try block)
• If PIT exists and no cursor was created, call pit.delete()
• Wrap pit.delete() in try-catch to handle RuntimeException (following pattern from ElasticJoinExecutor)
• On deletion error, log the error and increment the FAILED_REQ_COUNT_SYS metric (matching existing pattern)
• Do NOT delete PIT if cursor was created, as the cursor will manage PIT lifecycle through subsequent pagination requests and cursor close operations

Task 3: Handle Exception Path with PIT Cleanup

Ensure PIT is cleaned up when exceptions occur during query execution.

• Add catch block before finally to handle any exceptions during search execution or cursor creation
• In catch block, if PIT was created (pit != null), attempt to delete it with error handling
• Re-throw the original exception after cleanup attempt
• Follow the same error logging and metrics pattern as the finally block cleanup
• This ensures PITs don't leak when queries fail or are interrupted

🤖 Prompt for AI agents

Add PIT lifecycle management with try-finally blocks to ensure proper cleanup
when PIT is created but cursor is not generated.

In buildProtocolForDefaultQuery():
- Add boolean flag to track if cursor was created (initialized to false)
- Wrap search execution and cursor creation in try-catch-finally block
- In try block: execute search, check if cursor needed, create cursor if needed
and set flag to true
- In catch block: if pit != null, attempt pit.delete() with error handling, then
re-throw exception
- In finally block: if pit != null and cursor was NOT created, call pit.delete()
wrapped in try-catch
- On deletion errors, log and increment FAILED_REQ_COUNT_SYS metric
- Keep Protocol creation and return after try-finally block

Phase 3: Update Tests for PIT Lifecycle Management

Add test coverage for the new conditional PIT creation and cleanup logic to ensure PITs are only created when needed and properly cleaned up in all code paths.

Task 1: Add Tests for Non-Cursor Queries

Create or update tests in legacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorTest.java to verify PIT is NOT created for single-page queries.

• Add test case for query with fetch_size=0 that verifies PointInTimeHandler is never instantiated
• Add test case for query without fetch_size specified that verifies no PIT creation
• Mock the Client and verify createPit() is never called on the client
• Ensure search executes successfully without PIT for these scenarios

Task 2: Add Tests for Cursor Queries with PIT

Add test cases to verify PIT is created correctly when cursor is requested and managed properly throughout pagination lifecycle.

• Add test case for query with fetch_size > 0 that verifies PointInTimeHandler is created
• Mock Client.createPit() to return a test PIT ID
• Verify PIT ID is set on SearchRequestBuilder before execution
• When total hits >= fetch_size (cursor created), verify PIT is NOT deleted (cursor owns the lifecycle)
• When total hits < fetch_size (no cursor needed), verify PIT is deleted in finally block

Task 3: Add Tests for Error Path Cleanup

Add test cases to verify PIT cleanup occurs when exceptions are thrown during query execution.

• Add test case where search execution throws exception after PIT creation
• Verify PIT is deleted even when exception occurs
• Add test case where cursor creation logic throws exception after PIT creation
• Verify PIT deletion error handling logs properly and doesn't suppress original exception
• Mock metrics to verify FAILED_REQ_COUNT_SYS is incremented when PIT deletion fails

🤖 Prompt for AI agents

Add comprehensive test coverage for conditional PIT creation and cleanup logic.

In PrettyFormatRestExecutorTest:
- Add tests for fetch_size=0 and unspecified fetch_size verifying no PIT
creation
- Add tests for fetch_size > 0 verifying PIT is created and set on
SearchRequestBuilder
- Add test for cursor created scenario verifying PIT is NOT deleted
- Add test for no cursor scenario verifying PIT IS deleted
- Add tests for exception during search execution verifying PIT cleanup
- Add tests for exception during cursor creation verifying PIT cleanup
- Verify error logging and FAILED_REQ_COUNT_SYS metric increments on cleanup
failures
- Mock Client, SearchRequestBuilder, SearchResponse, and metrics as needed

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Modify PrettyFormatRestExecutor.buildProtocolForDefaultQuery() to conditionally
create PIT only when cursor-based pagination is needed.

In buildProtocolForDefaultQuery():
- Declare PointInTimeHandler variable initialized to null
- Check if queryAction.getSqlRequest().fetchSize() > 0
- Only create PointInTimeHandlerImpl and call pit.create() when fetchSize > 0
- Only call setPointInTime on SearchRequestBuilder when PIT is created
- Execute search request normally whether PIT is created or not
===============================================================================

Task: 2

Add PIT lifecycle management with try-finally blocks to ensure proper cleanup
when PIT is created but cursor is not generated.

In buildProtocolForDefaultQuery():
- Add boolean flag to track if cursor was created (initialized to false)
- Wrap search execution and cursor creation in try-catch-finally block
- In try block: execute search, check if cursor needed, create cursor if needed
and set flag to true
- In catch block: if pit != null, attempt pit.delete() with error handling, then
re-throw exception
- In finally block: if pit != null and cursor was NOT created, call pit.delete()
wrapped in try-catch
- On deletion errors, log and increment FAILED_REQ_COUNT_SYS metric
- Keep Protocol creation and return after try-finally block
===============================================================================

Task: 3

Add comprehensive test coverage for conditional PIT creation and cleanup logic.

In PrettyFormatRestExecutorTest:
- Add tests for fetch_size=0 and unspecified fetch_size verifying no PIT
creation
- Add tests for fetch_size > 0 verifying PIT is created and set on
SearchRequestBuilder
- Add test for cursor created scenario verifying PIT is NOT deleted
- Add test for no cursor scenario verifying PIT IS deleted
- Add tests for exception during search execution verifying PIT cleanup
- Add tests for exception during cursor creation verifying PIT cleanup
- Verify error logging and FAILED_REQ_COUNT_SYS metric increments on cleanup
failures
- Mock Client, SearchRequestBuilder, SearchResponse, and metrics as needed

💡 Iterate on the plan with: @coderabbitai <feedback>

Example Feedback
- @coderabbitai You can skip phase 3. Add a simple unit test case for phase 2.
- @coderabbitai For assumption 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!

[aalva500-prog]

Hi @dai-chen, I was able to reproduce the bug, but only when the index name contains periods like test-logs-2025.01.01. It doesn't happen with hyphens like test-logs-2025-01-01.