Skip to content

fix(database): correct statement timeout to MariaDB seconds (10s) - #26

Merged
shikendon merged 1 commit into
masterfrom
fix/statement-timeout-seconds
Jul 7, 2026
Merged

fix(database): correct statement timeout to MariaDB seconds (10s)#26
shikendon merged 1 commit into
masterfrom
fix/statement-timeout-seconds

Conversation

@clawd131662

@clawd131662 clawd131662 Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Problem

config/database.yml sets a per-connection statement timeout:

init_command: SET max_statement_time = <%= ENV.fetch('TIMEOUT', 1000) %>

In #16 this was ported from MySQL's MAX_EXECUTION_TIME (milliseconds) to MariaDB's max_statement_time (seconds), but the numeric values were carried over unchanged. The target DB is MariaDB 10.11.x, so the units are wrong by 1000×:

Setting Value MySQL intent (ms) Actual on MariaDB (s)
config/database.yml default 1000 1 s 1000 s (~16.7 min)
.env.template TIMEOUT 10000 10 s 10000 s (~2.8 h)

Confirmed via git log: commit 2689aa6 (#16) changed SET MAX_EXECUTION_TIMESET max_statement_time while keeping ENV.fetch('TIMEOUT', 1000). Per the MariaDB docs, max_statement_time is in seconds (a double); MySQL's max_execution_time is the millisecond one.

Scope difference (why not 1 s)

MariaDB's max_statement_time applies to all statements including DELETE; MySQL's max_execution_time only governed read-only SELECTs. So this fix makes the purge delete_all/destroy_all batches subject to the timeout for the first time — previously they were never timed out (SELECT-only on MySQL, then masked by the 1000× units on MariaDB).

Because the tool's core workload is 1000-row batched deletes on large multi-index tables (co_block, co_container ~60GB), a 1 s cap could abort batches mid-purge. So both values are set to 10 s (headroom for the DELETE workload, while still catching runaway queries).

Fix

  • config/database.yml default: 100010
  • .env.template TIMEOUT: 1000010

Follow-up (not in this PR)

The live .env on the host is git-ignored and still has TIMEOUT=10000 (~2.8 h effective cap). Update it to 10 and reconnect for the guard to take effect in production. Worth confirming empirically that a batch delete comfortably fits within 10 s on the production tables before relying on it.

Testing

Config-only change; no Ruby touched (Rubocop/tests N/A). YAML/ERB still renders — trivial numeric edit.

🤖 Generated with Claude Code

Comment thread config/database.yml Outdated
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review summary

The core fix is correct. I independently confirmed the units: MariaDB's max_statement_time is measured in seconds (a double), whereas MySQL's max_execution_time was milliseconds — so #16's straight port left the guard off by 1000×. Converting 1000 → 1 and 10000 → 10 faithfully restores the original 1 s / 10 s intent. The diff is minimal, config-only, and the ERB still renders.

I left one non-blocking inline note on config/database.yml: this change makes the statement timeout actually enforced against the purge DELETEs for the first time — MySQL's max_execution_time was SELECT-only, and #16's wrong units masked it since. Given the tool's workload is batched deletes on the large multi-index tables, it's worth confirming a batch fits within 10 s in production and reconsidering whether a 1 s fallback default is safe when TIMEOUT is unset.

The PR description's "Note (not in this PR)" about updating the live git-ignored .env (still TIMEOUT=10000) is the right call-out — please don't lose track of it, since the fix has no effect in prod until that connection is refreshed.

Nice, well-documented catch. 👍

@clawd131662 clawd131662 Bot changed the title fix(database): correct statement timeout to MariaDB seconds fix(database): correct statement timeout to MariaDB seconds (10s) Jul 7, 2026
@shikendon
shikendon force-pushed the fix/statement-timeout-seconds branch from c7df7fb to 471441c Compare July 7, 2026 16:41
Comment thread config/database.yml
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review summary

The core units fix is correct. I re-verified independently: MariaDB's max_statement_time is expressed in seconds (a double), whereas MySQL's max_execution_time was milliseconds — so #16's straight port of 1000/10000 left the guard off by 1000× (a ~16.7 min default and a ~2.8 h .env.template cap). Setting both to 10 restores a sane 10 s bound. The diff is minimal and config-only; the ERB still renders.

I also confirmed the scope claim in the description: MySQL's max_execution_time applied to read-only SELECTs, while MariaDB's max_statement_time applies to writes too — so this is genuinely the first time the purge DELETE batches are subject to the timeout.

Prior thread resolved: my earlier review flagged that a 1 s fallback default was risky for a DELETE-heavy tool. That was addressed here by raising the default to 10, so I've resolved that thread.

Remaining, non-blocking:

  • The description already calls it out, but it bears repeating as a pre-merge operational check: confirm a 1000-row batch delete comfortably fits within 10 s on the production tables (co_block with its extra indexes, co_container ~60GB) — otherwise batches will start aborting once the guard becomes real. A timed-out single delete_all statement rolls back atomically (no partial batch), but scripts/purge.rb has no rescue, so it would crash mid-run.
  • Don't lose the follow-up on the git-ignored live .env (still TIMEOUT=10000) — the fix has no prod effect until that connection is refreshed.
  • One small inline nit: encode the unit ("seconds") in a comment next to the setting so this 1000× class of mistake can't silently recur.

Nice, well-documented catch. 👍

`init_command` was ported from MySQL's `MAX_EXECUTION_TIME` (milliseconds)
to MariaDB's `max_statement_time` (seconds) in #16, but the numeric values
were carried over unchanged, leaving the guard off by 1000x (a 1000s default
and 10000s / ~2.8h in `.env.template`).

MariaDB's `max_statement_time` also differs from MySQL in scope: it applies
to all statements including DELETE, whereas MySQL's `max_execution_time`
governed read-only SELECTs only. So this is the first time the purge DELETE
batches are actually subject to the timeout. A 1s cap would abort 1000-row
batch deletes on the large multi-index tables (`co_block`, `co_container`),
so use 10s — headroom for the DELETE workload while still catching runaway
queries:

- config/database.yml default: 1000 -> 10
- .env.template TIMEOUT:      10000 -> 10

Also annotate the unit inline in both files so the ms/seconds confusion
that caused this cannot silently recur.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@shikendon
shikendon force-pushed the fix/statement-timeout-seconds branch from 471441c to 42a7ec5 Compare July 7, 2026 16:46
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review summary

The fix is correct and I have nothing new worth changing in the diff.

  • Verified the core claim: MariaDB max_statement_time is expressed in seconds (a double), whereas MySQL's max_execution_time was milliseconds. Upgrade to run on Debian 12 Ruby 3.1.2 #16's straight port of 1000/10000 therefore left the guard off by 1000× (~16.7 min default, ~2.8 h in .env.template). Setting both to 10 restores a sane 10 s bound, and the inline unit comments in both files are good insurance against this class of mistake recurring.
  • The scope note is also right: MariaDB's max_statement_time applies to writes (DELETE) too, unlike MySQL's SELECT-only max_execution_time — so this is genuinely the first time the purge batches are subject to the timeout.

Prior threads resolved: the 1 s fallback-default concern (addressed by raising the default 1 → 10) and the "encode the unit in a comment" nit (both files now carry it).

Non-blocking, operational (already acknowledged in the description):

  • Confirm a 1000-row batch delete comfortably fits within 10 s on the production tables (co_block with its extra data/user_action indexes, co_container ~60GB) before relying on the guard. Note scripts/purge.rb has no rescue, so a timed-out delete_all/destroy_all would raise ActiveRecord::StatementInvalid and abort the run mid-purge — safe (each statement rolls back atomically) but abrupt.
  • The follow-up on the git-ignored live .env (still TIMEOUT=10000) stands — the fix has no prod effect until that connection is refreshed.

Nice, well-documented catch. 👍

@shikendon
shikendon merged commit 8e9a659 into master Jul 7, 2026
1 check passed
@shikendon
shikendon deleted the fix/statement-timeout-seconds branch July 7, 2026 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants