kevinjqliu commented on issue #1793: URL: https://github.com/apache/iceberg-go/issues/1793#issuecomment-5615018228
Some context from digging into this over in apache/iceberg#18043. The test already runs `PRAGMA journal_mode=WAL` before it starts, so WAL isn't the missing piece. WAL only lets readers run alongside a writer; two writers still contend, and the loser gets `SQLITE_BUSY` immediately unless there's a busy timeout. The go sqlite drivers default `busy_timeout` to 0. (sqlite-jdbc defaults to 3s, which is why the java side gets away with WAL alone.) The package doc on `NewCatalog` already says sqlite callers with concurrent writers should set WAL and a busy timeout in the DSN, and `isRetryableSerializableError` treats "database is locked" as retryable. So the catalog is doing the right thing, the test just isn't configured for it. My preference is to set the busy timeout in the test DSN rather than loosen the assertion. Two things to watch: - running `PRAGMA busy_timeout` on one connection isn't enough, `database/sql` pools connections and the pragma is per-connection. It needs to be in the DSN (or in a connect hook) so every pooled connection gets it. - `sqliteshim` picks mattn under cgo and modernc without it, and CI runs both. The DSN syntax differs: mattn wants `_busy_timeout=5000`, modernc wants `_pragma=busy_timeout(5000)`. The test needs to handle both, or open the db itself and hand it to `NewCatalog`. `_txlock=immediate` would also work but it's mattn-only afaik, so busy_timeout is the more portable option. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
