Cache PreparedStatements for all SQL issued by INDEXED files - #894
Open
yutaro-sakamoto wants to merge 13 commits into
Open
Cache PreparedStatements for all SQL issued by INDEXED files#894yutaro-sakamoto wants to merge 13 commits into
yutaro-sakamoto wants to merge 13 commits into
Conversation
codegen.c kept the inside_check/inside_stack bookkeeping from the original C code generator, but only the decrements survived the rewrite to Java code generation: nothing ever increments inside_check. On non-GCC builds (the MSVC-built cobj.exe) the counter goes negative as soon as a runtime check is attached to a reference (-debug with a variable subscript, reference modification or SEARCH ALL), and inside_stack[inside_check - 1] then writes out of bounds, crashing the compiler. GCC builds never compile these blocks, which is why Linux was unaffected. Remove the machinery entirely: the generated Java must not depend on the compiler that built cobj. Un-skip the Windows tests that were failing due to this crash. Fixes opensourcecobol#829, opensourcecobol#831, opensourcecobol#832, opensourcecobol#833, opensourcecobol#834 (sub-issues of opensourcecobol#828)
With -java-package the jar arguments were assembled as package_dir + file_path_delimitor + class, mixing the '/' separators from the package path with '\' on Windows (e.g. com/abc\prog.class), which produces malformed entry names in the archive. The jar tool accepts '/' everywhere, so join the paths with '/' unconditionally. The class file cleanup after archiving had the opposite problem: cmd.exe's del does not accept '/' separators. Build the remove paths with the native separator, let cd handle the output directory, and drop a leftover '#aaa' token that del would treat as an argument. Un-skip the Windows jar test. Fixes opensourcecobol#830 (sub-issue of opensourcecobol#828)
Removing the inside_check block exposed a variableScope finding for 'code' in joutput_stmt; declare it in the handler block where it is used.
Every program in a translation unit becomes its own .java/.class file named after the program ID, so two programs whose IDs differ only in letter case overwrite each other's files on a case-insensitive filesystem and the result silently misbehaves. On Windows, detect the collision before code generation and fail with an explicit error. The two affected tests stay skipped on Windows because the behavior they exercise is impossible there by design; a new Windows-only test verifies the diagnostic instead. Fixes opensourcecobol#835 (sub-issue of opensourcecobol#828)
On case-insensitive file systems programs whose PROGRAM-IDs differ only in letter case would generate class files with the same name; cobj now reports a compile error there, so document the restriction in the usage section.
* Use "cd /d" on Windows: cmd.exe's plain "cd" does not switch drives, so with -o pointing at another drive the jar and del commands would run against the wrong directory * Replace the unbounded strcpy into package_dir_native with snprintf and give package_name_to_path an explicit output bound, so an overlong -java-package argument can no longer overflow the buffers
…every WRITE An indexed file opened with OUTPUT holds an exclusive file lock and only WRITE statements run until CLOSE, so no other process can observe the intermediate states. Committing the JDBC transaction - and therefore fsyncing the SQLite database - after every WRITE is unnecessary there. The records are now committed by the COBOL COMMIT statement and at CLOSE. Writing 100,000 records takes 0.46s instead of 382.7s. COB_FILE_IDX_COMMIT_INTERVAL makes the runtime commit on its own every N successful WRITEs as well, which bounds what a crash before CLOSE can lose at N records in exchange for one fsync per interval. It defaults to INF (no intermediate commits) because COBOL has a COMMIT statement, so the commit points belong to the program. 0 is treated as 1 (commit per WRITE) and an invalid value falls back to the default with a warning. Duplicate-key detection is unaffected: uncommitted rows are visible to queries on the same connection, so WRITE still reports file status 21 and 22 immediately. Each WRITE of a file with alternate keys runs inside a savepoint so that a failed WRITE rolls back only its own changes; files with no alternate key need no savepoint because their WRITE is a single atomic INSERT. Supporting changes: - exitFileIO now really closes the files it warns about. It only printed "WARNING - Implicit CLOSE" before, so a program reaching STOP RUN without CLOSE lost the deferred records and left a stale file_lock row that made every later OPEN fail with status 61. - unlock_ releases this process's record locks instead of printing "Unlocking INDEXED file is not implemented", so UNLOCK, COMMIT and ROLLBACK work on indexed files. The file_lock row is deliberately kept until CLOSE, since it doubles as the open registration. - ROLLBACK is dispatched through a rollback_ hook. For a file open OUTPUT it discards the writes that are still uncommitted; in the other modes every statement is already committed, so it releases the record locks only, as cob_rollback does in opensource COBOL 1.x. - SQL failures on the WRITE path used to be reported as status 51 (record locked) whatever went wrong. They now map to 34 for disk full, 22 for a constraint violation, 51 for lock contention and 30 otherwise. CLOSE reports 30 when a failed WRITE may have taken the buffered records with it, since SQLite rolls back the whole transaction for conditions such as disk full. - The file cache keeps only open files, and the INSERT statements are prepared once per table instead of on every WRITE.
Extend the existing per-open INSERT statement cache to cover every SQL statement issued by the INDEXED file implementation. A new class IndexedStatementCache keys PreparedStatements by SQL text and lives as long as the connection, so hot paths (cursor reads, key-existence checks, record locking, delete internals) no longer re-parse the same SQL on every COBOL I/O verb. Also fix small resource issues found on the way: an unclosed ResultSet in checkVersionOld, a leaked PreparedStatement and a missing rs.next() check in getNextKeyDupNo, and an unused Statement in close_.
fetchFirstRecord and fetchLastRecord run once per scan and bind no parameters, so caching a PreparedStatement for them buys nothing and contradicts the rule the cache documents: one-shot statements stay on try-with-resources. The cursor now holds the IndexedFile, which gives it both the connection for those two queries and the cache for the per-record ones. Drop insertStatementFor as well. Now that the INSERT statements come from the shared cache, the wrapper only forwarded to statementCache.get(), and inlining it makes the WRITE path read like every other caller.
yutaro-sakamoto
marked this pull request as ready for review
August 21, 2026 10:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
このPRは #892 の上に作られている。#892 のコミットを含むため、#892 をマージした後にレビュー・マージしてほしい。このPR自身の変更は末尾の3コミット(
perf: cache PreparedStatements for all INDEXED file SQL以降)だけである。#892 で「INSERT文はテーブルごとに1度だけ準備するようにした」と書いたが、本PRはその考え方をINDEXEDファイルが発行するSQL全体へ広げるものである。
概要
INDEXEDファイルの実装では、#892 の時点で INSERT 文だけが
PreparedStatementとしてキャッシュされていた。それ以外のSQL(カーソルによる READ、キー存在確認、レコードロック、DELETE/REWRITE の内部処理)は、COBOLのI/O文を実行するたびにprepareStatementからやり直しており、1レコード読むごとに同じSQLを解析し直していた。そこで、SQL文字列をキーに
PreparedStatementをキャッシュする専用クラスIndexedStatementCacheを新設し、INDEXEDファイルが繰り返し発行するSQLをそこへ通すようにした。IndexedFile.statementCacheが保持し、接続の確立時に生成、CLOSE時にcloseAll()でまとめて解放する。IndexedFile.cachedInsertStatementsとcloseCachedInsertStatements)は、この新しいキャッシュに統合して削除した。INSERT文も他の箇所と同じくstatementCache.get(...)で取得する形に揃え、ラッパだったinsertStatementForも廃止した。IndexedCursorはConnectionの代わりにIndexedFileを受け取るようにした。レコードごとに実行されるパラメータ付きのクエリはキャッシュから取得し、fetchFirstRecord/fetchLastRecordは走査1回につき1度しか実行されずパラメータも持たないため、素のStatementのままとした。try-with-resources に置いている。SQL文字列をキーにしてよい理由は、SQL文がすべて定数テンプレートとテーブル名から組み立てられ、値は必ずプレースホルダでバインドされるため、文字列の種類が有界で同じ文字列が常に同じ意味を持つことによる。
PreparedStatementは接続内で完結するので、ファイルを他プロセスと共有していても、コミット・ロールバック・セーブポイントをまたいでも再利用して安全である(#892 のINSERTキャッシュと同じ根拠)。ついでに、作業中に見つかった資源の後始末の不備も修正した。
checkVersionOldでResultSetが閉じられていなかったgetNextKeyDupNoでPreparedStatementが閉じられておらず、またrs.next()を呼ばずにgetIntしていた(未使用の引数も削除)close_で生成だけされて使われていないStatementがあった性能
主キーと重複ありの副キーを持つ50,000件のファイルに対して計測した(4回実行し、ウォームアップの初回を除いた3回の中央値)。比較対象は本PRの3コミットの有無のみで、いずれも #892 を含むビルドである。
更新系(REWRITE/DELETE)は実ディスク上ではI/O文ごとの
fsyncが支配的で、SQLの解析コストが埋もれてしまう。そこで実ディスク(ext4)に加えて、fsyncが事実上無視される tmpfs 上でも計測し、SQL処理そのものへの効果を切り分けた。tmpfs(fsyncの影響を除去し、SQL処理コストを分離)
ext4(実ディスク、エンドツーエンド)
読み取りの多い経路が最も効き、順次走査で 1.7 倍、主キーのランダムREADで 1.2〜1.35 倍speedupした。DELETE はSQL処理だけを見れば 1.56 倍だが、実ディスクでは
fsyncが支配的でエンドツーエンドの効果は小さくなる(更新系のフェーズは実行ごとのばらつきも 1 割程度ある)。REWRITE の伸びが小さいのは、重複キーの採番で実行されるselect ifnull(max(dupNo), -1) from tableNがdupNoに索引を持たず全走査になるという、本変更とは別の既存のボトルネックが支配的なためである(この点は別途対応したい)。テスト
CIは全ジョブ成功(NIST IX を含む cobol85、file-lock〜file-lock5、cobj-idx、misc、run など)。加えてローカルで、変更前後のビルドの出力が一致することを次のプログラムで確認した。
STARTを伴わないREAD NEXT(fetchFirstRecordを通る経路)と、EOF到達後のREAD PREVIOUS(fetchLastRecordを通る経路)Important
This PR is built on top of #892 and contains its commits, so please review and merge it after #892. Only the last three commits (from
perf: cache PreparedStatements for all INDEXED file SQLonwards) belong to this PR.#892 noted that the INSERT statements are prepared once per table; this PR extends that idea to all of the SQL the INDEXED file implementation issues.
Summary
As of #892, only INSERT statements were cached as
PreparedStatements in the INDEXED file implementation. Every other statement — cursor reads, key-existence checks, record locking and the DELETE/REWRITE internals — was re-created fromprepareStatementon every COBOL I/O verb, so the same SQL was parsed again for every record read.This adds
IndexedStatementCache, a dedicated class that cachesPreparedStatements keyed by SQL text, and routes the repeatedly issued INDEXED file SQL through it.IndexedFile.statementCache, created when the connection is established and released withcloseAll()at CLOSE.IndexedFile.cachedInsertStatementsandcloseCachedInsertStatements) is folded into it and removed. INSERT statements are now fetched withstatementCache.get(...)like every other statement, so theinsertStatementForwrapper is gone too.IndexedCursortakes theIndexedFileinstead of aConnection. The parameterized queries it runs per record come from the cache, whilefetchFirstRecord/fetchLastRecordkeep a plainStatement: they run once per scan and bind no parameters, so caching them would buy nothing.Keying on the SQL text is safe because every statement is built from constant templates plus a table name, with all values bound through placeholders: the set of strings is bounded and a given string always means the same thing.
PreparedStatements are scoped to their connection, so reusing them across commits, rollbacks and savepoints is safe even when the file is shared with other processes — the same reasoning #892 relied on for the INSERT cache.A few resource-handling defects found along the way are fixed as well: an unclosed
ResultSetincheckVersionOld, a leakedPreparedStatementplus a missingrs.next()check ingetNextKeyDupNo(whose unused parameter is also dropped), and an unusedStatementinclose_.Performance
Measured on a 50,000-record file with a primary key and an alternate key with duplicates (median of 3 runs after discarding a warm-up run). The only difference between the two builds is the three commits of this PR; both include #892.
On a real disk the update paths are dominated by the
fsynceach I/O statement performs, which buries the SQL parsing cost. The benchmark was therefore run both on ext4 and on tmpfs, wherefsyncis effectively free, to separate the effect on the SQL work itself.tmpfs (fsync removed, isolating the SQL cost)
ext4 (real disk, end to end)
Read-heavy paths benefit most: 1.7x on a sequential scan and 1.2x to 1.35x on random reads by primary key. DELETE is 1.56x faster in SQL terms, but on disk
fsyncdominates and the end-to-end gain shrinks (the update phases also vary by around 10% run to run). REWRITE gains little because it is dominated by an unrelated, pre-existing bottleneck: numbering a duplicate key runsselect ifnull(max(dupNo), -1) from tableN, which has no index ondupNoand therefore scans the whole table. That deserves a separate fix.Testing
CI is green on every job (cobol85 including NIST IX, file-lock through file-lock5, cobj-idx, misc, run and the rest). Locally, the builds before and after the change produce identical output for:
READ NEXTwithout a precedingSTART(thefetchFirstRecordpath) andREAD PREVIOUSafter reaching EOF (thefetchLastRecordpath).