You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "xtern (via GitHub)" <gi...@apache.org> on 2025/11/27 11:40:25 UTC
[PR] IGNITE-27117 In a multistatement query if an error occurs do not abort preceding statements [ignite-3]
xtern opened a new pull request, #7099:
URL: https://github.com/apache/ignite-3/pull/7099
https://issues.apache.org/jira/browse/IGNITE-27117
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27117 Don't abort execution of previous statements when an error occurs in a multi-statement query [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7099:
URL: https://github.com/apache/ignite-3/pull/7099#discussion_r2568824995
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItSqlMultiStatementTest.java:
##########
@@ -207,6 +208,43 @@ void scriptStopsExecutionOnError() {
}
}
+ /**
+ * The test verifies that if an error occurs within the script,
+ * previous statements will not be forcibly cancelled.
+ *
+ * <p>Note: this behavior is important for {@code JdbcStatement.executeBatch},
+ * because the method executes a batch of statements as a script,
+ * and if an error occurs somewhere, it is impossible to correctly count
+ * the number of updates (for a batch update exception).
+ */
+ @Test
+ void precedingStatementsDontAbortedOnError() throws InterruptedException {
Review Comment:
The method name has a grammatical error. Consider renaming to `precedingStatementsAreNotAbortedOnError` or `precedingStatementsDontGetAbortedOnError` for correct grammar. The current name mixes "don't" (a contraction) with "aborted" (past participle) incorrectly.
```suggestion
void precedingStatementsAreNotAbortedOnError() throws InterruptedException {
```
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItSqlMultiStatementTest.java:
##########
@@ -207,6 +208,43 @@ void scriptStopsExecutionOnError() {
}
}
+ /**
+ * The test verifies that if an error occurs within the script,
+ * previous statements will not be forcibly cancelled.
+ *
+ * <p>Note: this behavior is important for {@code JdbcStatement.executeBatch},
+ * because the method executes a batch of statements as a script,
+ * and if an error occurs somewhere, it is impossible to correctly count
+ * the number of updates (for a batch update exception).
+ */
+ @Test
+ void precedingStatementsDontAbortedOnError() throws InterruptedException {
+ String script = "CREATE TABLE test (id INT PRIMARY KEY, val INT);"
+ + "INSERT INTO test VALUES (0, 0), (1, 1), (2, 2);"
+ + "UPDATE test SET val = val + 1;"
+ + "INSERT INTO test VALUES (3, 3/0);";
+
+ AsyncSqlCursor<InternalSqlRow> ddlCursor = runScript(script);
+ AsyncSqlCursor<InternalSqlRow> dmlCursor1 = await(ddlCursor.nextResult());
+ AsyncSqlCursor<InternalSqlRow> dmlCursor2 = await(dmlCursor1.nextResult());
+
+ assertThrowsSqlException(
+ RUNTIME_ERR,
+ "Division by zero",
+ () -> await(dmlCursor2.nextResult())
+ );
+
+ // Put some delay to ensure no one tries to close the cursors in the background.
+ Thread.sleep(100);
+
+ validateSingleResult(ddlCursor, true);
+ validateSingleResult(dmlCursor1, 3L);
+ validateSingleResult(dmlCursor2, 3L);
+
+ List.of(ddlCursor, dmlCursor1, dmlCursor2)
+ .forEach(AsyncCursor::closeAsync);
Review Comment:
The cursors are being closed twice. The `validateSingleResult` method (line 240-242) already calls `cursor.closeAsync()` on each cursor (see BaseSqlMultiStatementTest.java:186), so calling `closeAsync()` again here is redundant and could cause issues. Remove these explicit close calls since they're already handled by `validateSingleResult`.
```suggestion
// Cursors are already closed by validateSingleResult above.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27117 Don't abort execution of previous statements when an error occurs in a multi-statement query [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern merged PR #7099:
URL: https://github.com/apache/ignite-3/pull/7099
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27117 Don't abort execution of previous statements when an error occurs in a multi-statement query [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #7099:
URL: https://github.com/apache/ignite-3/pull/7099#discussion_r2579864212
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/tx/ScriptTransactionWrapperImpl.java:
##########
@@ -94,6 +98,17 @@ public CompletableFuture<Void> finalise(Throwable error) {
rollbackCause = error;
txState = State.ROLLBACK;
+
+ cursorsToClose = List.copyOf(openedCursors.values());
+ }
+
+ // Close all associated cursors on error.
+ for (CompletableFuture<? extends AsyncCursor<?>> fut : cursorsToClose) {
+ fut.whenComplete((cursor, ex) -> {
+ if (cursor != null) {
+ cursor.closeAsync();
Review Comment:
it's better to use `cursor.cancelAsync(CancellationReason.CANCEL)`.
Btw, does it make sense to wait for completion before terminating the transaction?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org