Firebird: Handle COMMIT/ROLLBACK via transaction manager API in script execution by fdcastel · Pull Request #40665 · dbeaver/dbeaver

fdcastel · 2026-04-01T03:52:24Z

Problem

When executing SQL scripts via Alt+X (Execute SQL Script), COMMIT and ROLLBACK statements are sent as raw SQL to the database. This causes failures on databases where the JDBC driver manages transactions exclusively through the JDBC API rather than accepting raw transaction control SQL.

For example, for the following SQL

See attached pictures. They show the execution result of DBeaver "Execute SQL Script" command (`Alt-X`) for the following script on a Firebird database:
```sql
RECREATE TABLE TEST_SECTION9 (
    ID INTEGER NOT NULL,
    NAME VARCHAR(100),
    CATEGORY VARCHAR(50),
    AMOUNT DECIMAL(12,2),
    CREATED_AT TIMESTAMP,
    BIN_VALUE BIGINT,
    CONSTRAINT PK_TEST_SECTION9 PRIMARY KEY (ID)
);

COMMIT;

INSERT INTO TEST_SECTION9 (ID, NAME, CATEGORY, AMOUNT, CREATED_AT, BIN_VALUE) VALUES
    (1, 'Alice', 'A', 10.00, CURRENT_TIMESTAMP, 1);
INSERT INTO TEST_SECTION9 (ID, NAME, CATEGORY, AMOUNT, CREATED_AT, BIN_VALUE) VALUES
    (2, 'Bob', 'A', 15.00, CURRENT_TIMESTAMP, 2);
INSERT INTO TEST_SECTION9 (ID, NAME, CATEGORY, AMOUNT, CREATED_AT, BIN_VALUE) VALUES
    (3, 'Carol', 'B', 7.50, CURRENT_TIMESTAMP, 4);
INSERT INTO TEST_SECTION9 (ID, NAME, CATEGORY, AMOUNT, CREATED_AT, BIN_VALUE) VALUES
    (4, 'Dave', 'B', 22.00, CURRENT_TIMESTAMP, 8);

COMMIT;

Firebird fails with:

SQL Error [335544332] [08003]: invalid transaction handle (expecting explicit transaction start)

in both auto-commit and manual commit modes.

Root Cause

In SQLQueryJob.executeSingleElement(), the ExecuteStatement() method sends COMMIT/ROLLBACK as raw SQL before the existing handleTransactionStatements() logic can handle them via the transaction manager API. The raw SQL execution fails for databases like Firebird, so the transaction manager handling is never reached.

Fix

Intercept COMMIT/ROLLBACK statements at the beginning of ExecuteSingleElement() and return early, delegating transaction handling to the existing handleTransactionStatements() call in the script execution loop. This correctly:

Auto-commit mode: Ignores the statement (it is a no-op)
Manual commit mode: Commits/rolls back via DBCTransactionManager API

Testing

All existing unit tests pass (cd test && mvn clean verify)
Tested with Firebird database in both auto-commit and manual commit modes

This PR was generated with AI (GitHub Copilot), supervised and tested by @fdcastel.

fdcastel · 2026-04-01T04:03:14Z

Analysis of Cross-Database Impact

What the original code actually did (for databases like PostgreSQL/MySQL)

When encountering COMMIT in a script in manual commit mode, the original code did three things in sequence:

executeStatement() — sent COMMIT as raw SQL to the database
Inner handleTransactionStatements() (line 637) — called txnManager.commit() via JDBC API
Outer handleTransactionStatements() (line 260, in the main loop) — called txnManager.commit() again (double-commit)

So the original code was already broken for manual commit mode — it was committing twice via the JDBC API, plus once as raw SQL. That databases like PostgreSQL tolerated this is incidental.

What the fix does

With the fix, encountering COMMIT:

In manual commit mode: the outer loop's handleTransactionStatements() (line 260) calls txnManager.commit() exactly once via the JDBC API
In auto-commit mode: handleTransactionStatements() is never reached (condition !oldAutoCommit fails), so COMMIT is silently ignored — which is semantically correct since every statement auto-commits

Using connection.commit() is the JDBC standard and works correctly for all JDBC-compliant drivers, including PostgreSQL, MySQL, SQLite, etc.

The real concern: `isSupportsTransactions() == false`

There's one edge case: if a driver returns false from isSupportsTransactions(), neither handleTransactionStatements() call fires. With the fix, raw COMMIT SQL would then also be suppressed. But any driver reporting isSupportsTransactions() == false by definition doesn't support transactions, so sending raw COMMIT SQL to it would either be a no-op or an error anyway.

Test coverage

No unit tests exist for this code path at all — SQLQueryJob is a UI-layer class and the entire test suite uses in-memory mocks without real execution. The only live-database coverage is the Firebird UI tests.

There's a gap: no multi-database script execution test with COMMIT exists in the codebase for any database.

fdcastel · 2026-04-01T04:12:17Z

Moving this out of draft for now. Seeking for guidance.

IvanGorshechnikov · 2026-04-09T14:58:18Z

Your PR will broke other databases, where execute of COMMIT or ROLLBACK is valid operation

fdcastel · 2026-04-09T15:43:20Z

Your PR will broke other databases, where execute of COMMIT or ROLLBACK is valid operation

Hmm, I was under the impression that it wouldn’t, but I’m still learning.

Could you share some examples? Is there an existing test suite that covers this (or perhaps one I’m not running)?

I tried to run a quick check with DuckDB and SQLite, but both reject a COMMIT within a script like this:

SELECT 1;
COMMIT;
SELECT 2;

IvanGorshechnikov · 2026-04-09T18:00:54Z

Yes, but you mentioned PostgreSQL in your message

So the original code was already broken for manual commit mode — it was committing twice via the JDBC API, plus once as raw SQL. That databases like PostgreSQL tolerated this is incidental.
But it is not incidental; this is how PostgreSQL and its forks work. And it is very important to process these commands for it

fdcastel · 2026-04-09T18:17:26Z

Yes, but you mentioned PostgreSQL in your message

Sure. But I don't have one to test right now. I made this a week ago, in another system, far from where I am now.

I'm still unsure what you want me to do for this to go ahead. Please, help me to understand: what from the OP or its follow-up isn't clear?

There are no repro cases I can work against. There are no tests in the codebase for this. I may add them, but I will need a pointer.

Broadens scope from Firebird-only to multi-database so we can regression-guard upstream PRs that touch shared DBeaver internals. - CI: install Firebird via PSFirebird (New-FirebirdEnvironment + Start-FirebirdInstance) instead of the Inno-Setup silent installer; reuse the pre-installed PostgreSQL service on windows-latest runners. - New plugin local.dbeaver.postgresql.ui.test/ with a regression guard for dbeaver/dbeaver#40665 (script execution of COMMIT/ROLLBACK must keep working on databases that previously accepted raw COMMIT). - Refactor ConnectionUtil.createConnectionViaAPI to take a driver search term; add createPostgreSQLConnectionViaAPI wrapper. - Widen artifact and JUnit report globs to plugins/*/target/surefire-reports.

… #40665 - New plugin local.dbeaver.postgresql.ui.test/ mirroring the Firebird one. * PostgreSQLConnectionTest — creates a connection via the DBeaver API and asserts it appears in the navigator. * ScriptCommitRollbackTest — runs the repro script from dbeaver/dbeaver#40665 (DDL + INSERTs + COMMIT + ROLLBACK) via Alt+X and asserts no error dialog. Guards against the PR's COMMIT/ROLLBACK interception regressing on databases that historically accepted them as raw SQL. - Refactor support plugin's ConnectionUtil: extract createConnectionViaAPI (driverSearchTerm, displayName, ...) so Firebird and PostgreSQL can share the fast programmatic-registration path; updateConnectionViaAPI takes the search term too. - CI workflow now also starts the pre-installed PostgreSQL service on windows-latest (fastest option, no download), resets the postgres password, creates the ui_test database, and passes -Dpostgres.* alongside -Dfirebird.*. Artifact + JUnit-report globs widened to plugins/*/target/... so both test plugins are covered. Firebird install continues to use the official Inno-Setup installer unchanged. - README updated to describe the multi-database scope.

fdcastel · 2026-04-21T11:57:53Z

Hi team 👋 — this PR has been sitting idle for about two weeks since my last reply, and I wanted to share some new evidence that I hope helps move the review forward.

To rigorously address the concern that this change could break PostgreSQL (or other databases that historically accepted raw COMMIT / ROLLBACK in scripts), I put together a small SWTBot-based UI-test harness specifically for this PR — fdcastel/dbeaver-ui-test. It drives a real DBeaver Windows build and runs the exact repro script from the PR description against both Firebird and PostgreSQL, in both auto-commit and manual-commit mode.

Results against v17.0.0-pre.9 (the build with this PR merged — see below): 16/16 tests pass, 0 failures. The ones that matter for the review concern:

Scenario	Result
PostgreSQL, auto-commit (`ScriptCommitRollbackTest`)	✓
PostgreSQL, manual-commit (`ScriptCommitManualModeTest`)	✓
Firebird, auto-commit (`ScriptCommitRollbackTest`)	✓

The manual-commit variant is the one that actually exercises this PR's routing path, since in auto-commit mode COMMIT is a no-op on both the old and new code. It toggles the live PostgreSQL connection to manual commit via DBCTransactionManager.setAutoCommit(monitor, false), runs the PR's repro script (DDL + four INSERTs + two intercepted COMMITs), and then appends ROLLBACK; + SELECT COUNT(*) FROM test_section9_manual to the same script. If the intercepted COMMIT had silently failed (the exact regression I was asked about), the trailing ROLLBACK would discard the CREATE TABLE and the final SELECT would surface as an Execution Error dialog. The test passing proves the intercepted COMMIT really reached the server.

Screenshot artifacts from the run are attached to the CI job:

🔗 https://github.com/fdcastel/dbeaver-ui-test/actions/runs/24720390550
PASS_PR40665_Firebird_ScriptCommitRollback.png — Firebird, Statistics tab shows 7 queries / 4 rows
PASS_PR40665_PostgreSQL_ScriptCommitRollback.png — PostgreSQL auto-commit
PASS_PR40665_PostgreSQL_ManualCommit_ScriptCommitRollback.png — PostgreSQL manual-commit (the Commit/Rollback toolbar buttons are visible in the screenshot, confirming the mode)

For anyone who'd like to reproduce this locally or just try the fix, unofficial pre-release Windows/Linux/macOS builds with this PR merged are at:

🔗 https://github.com/fdcastel/dbeaver/releases — v17.0.0-pre.9 is the latest tag with this change.

If there is a concrete scenario where the new routing is expected to misbehave, I'd be very happy to turn it into a failing test case — the harness is designed so that adding one is a ~30-line test class. Just let me know what to look for.

Marking the PR as ready for review. Thanks for taking another look 🙏

When executing SQL scripts (Alt+X), COMMIT and ROLLBACK statements were sent as raw SQL to the database before being handled via the transaction manager API. This caused failures on databases where the JDBC driver manages transactions exclusively through the JDBC API (e.g., Firebird). Skip raw SQL execution for transaction control statements and delegate them to the existing transaction manager handling in the script execution loop, which correctly commits in manual mode and ignores in auto-commit. Assisted by: GitHub Copilot

fdcastel marked this pull request as draft April 1, 2026 04:01

fdcastel changed the title ~~Handle COMMIT/ROLLBACK via transaction manager API in script execution~~ Firebird: Handle COMMIT/ROLLBACK via transaction manager API in script execution Apr 1, 2026

fdcastel marked this pull request as ready for review April 1, 2026 04:11

E1izabeth added the external label Apr 1, 2026

fdcastel marked this pull request as draft April 9, 2026 15:39

fdcastel marked this pull request as ready for review April 21, 2026 11:57

fdcastel force-pushed the fix/sql-script-transaction-control branch from 5b90b8c to 4056936 Compare April 21, 2026 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firebird: Handle COMMIT/ROLLBACK via transaction manager API in script execution#40665

Firebird: Handle COMMIT/ROLLBACK via transaction manager API in script execution#40665
fdcastel wants to merge 1 commit intodbeaver:develfrom
fdcastel:fix/sql-script-transaction-control

fdcastel commented Apr 1, 2026 •

edited

Loading

Uh oh!

fdcastel commented Apr 1, 2026 •

edited

Loading

Uh oh!

fdcastel commented Apr 1, 2026

Uh oh!

IvanGorshechnikov commented Apr 9, 2026

Uh oh!

fdcastel commented Apr 9, 2026

Uh oh!

IvanGorshechnikov commented Apr 9, 2026

Uh oh!

fdcastel commented Apr 9, 2026

Uh oh!

fdcastel commented Apr 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

fdcastel commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root Cause

Fix

Testing

Uh oh!

fdcastel commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Analysis of Cross-Database Impact

What the original code actually did (for databases like PostgreSQL/MySQL)

What the fix does

The real concern: isSupportsTransactions() == false

Test coverage

Uh oh!

fdcastel commented Apr 1, 2026

Uh oh!

IvanGorshechnikov commented Apr 9, 2026

Uh oh!

fdcastel commented Apr 9, 2026

Uh oh!

IvanGorshechnikov commented Apr 9, 2026

Uh oh!

fdcastel commented Apr 9, 2026

Uh oh!

fdcastel commented Apr 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fdcastel commented Apr 1, 2026 •

edited

Loading

fdcastel commented Apr 1, 2026 •

edited

Loading

The real concern: `isSupportsTransactions() == false`