Repository navigation
Conversation
0f7a847 to
ec9be04
Compare
ec9be04 to
739bd01
Compare
29d0c9e to
ff70405
Compare
|
Rebased this one so it has a new |
ff70405 to
57507b7
Compare
57507b7 to
d2dd71a
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
d2dd71a to
c7cad01
Compare
c7cad01 to
1540484
Compare
6230b70 to
3466425
Compare
Here, add a new driver `rivermysql` that brings MySQL support to River. Similar to SQLite, it's unfortunately not quite as good as the Postgres driver, but it does the job. MySQL has more facilities than SQLite, but it's still missing some major niceties like `RETURNING`, which for many queries requires us to write an implementation using two operations -- one that performs the action, and then another that loads back the result using returned IDs. It also doesn't have listen/notify. Luckily, _like_ SQLite, any of this nastiness stays cordoned to the driver layer and doesn't leak into the mainline River code. We've got good driver testing and basic tests on clients for each driver in place so we get reasonable assurance that everything works. In the before times, I would've been fairly concerned at the additional maintenance burden that supporting another database would bring, but with the rise of LLMs I think it's more plausible that we can bring something like this in without much trouble. Incredibly, I was able to get almost all of this implemented in just one evening whereby my SQLite driver took me multiple months. I don't want to bring MySQL in as a hard dependency, so I've made the MySQL tests disabled by default. Use `RIVER_MYSQL_TESTS_ENABLED=true` to activate them. Only one CI matrix case runs MySQL tests so that we don't have to repeat them to exhaustion on every version of Go and Postgres. See also: #158
3466425 to
4d384a2
Compare
Sqlc is unsurprisingly ... buggy. Unfortunately though `JSON_TABLE` is technically supported in `master` right now, there's no release, and even having Codex try `master` it seems that it still doesn't work properly. Here, go around the use of sqlc for the couple major driver functions in MySQL that could use it to help perform a batch operation (for most driver operations it doesn't matter). For 1,000 row batches: | Operation | Before | After | Speedup | |---|---:|---:|---:| | Insert without returned rows | 579 ms | 17 ms | **33×** | | Complete/change state | 657 ms | 28 ms | **23×** | | Rescue | 599 ms | 22 ms | **27×** | | Notify | 457 ms | 6.8 ms | **68×** | Unfortunately it doesn't help with the standard bulk insert because we have to do something weird in MySQL to get each of the returned rows.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7930c2d085
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let polled = async { | ||
| let mut connection = connection?; | ||
| let receive = async { |
There was a problem hiding this comment.
When concurrent notification inserts reserve IDs and commit out of order, this pooled connection remains in autocommit mode while NotificationListener executes its FOR UPDATE/FOR SHARE reads. InnoDB does not retain the intended locking-read behavior without an explicit transaction, so the poll can observe a later committed ID, advance last_id, and permanently skip the earlier row when it commits. Begin and commit a transaction around each cursor/subscription/poll scan, as the other MySQL implementations in this commit do.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f829882c05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return nil, errJobListParamsMetadataNotSupportedSQLite | ||
| } | ||
| if c.driver.DatabaseName() == riverdriver.DatabaseNameMySQL && params.metadataCalled { | ||
| params = params.withMetadataPredicateSQL("JSON_CONTAINS(metadata, CAST(@metadata_fragment AS JSON))") |
There was a problem hiding this comment.
For nested arrays, JSON_CONTAINS does not preserve the containment semantics used by the other drivers: a stored fragment such as {"array":[[1]]} can satisfy a filter for {"array":[1]}, even though the nested level differs. Both JobList and JobListTx therefore return false-positive rows for valid metadata filters; use a structural predicate that traverses each object/array level, preferably through a driver/sqlc-backed query rather than embedding runtime SQL here.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| if (item.uniqueKey !== null) { | ||
| const key = Buffer.from(item.uniqueKey).toString("hex"); | ||
| if (keys.has(key)) | ||
| throw new ConfigurationError("Batch repeats a unique key"); | ||
| keys.add(key); |
There was a problem hiding this comment.
When multiple batch entries share a unique key but their uniqueStates do not include their insertion state, the functional River index evaluates to NULL and both rows are valid. This unconditional set check rejects such batches before insertion—for example, two pending jobs whose uniqueness applies only while available—while the other implementations only reject repeated keys whose current state is covered. Gate the check and insertion into keys on whether item.uniqueStates includes item.state.
Useful? React with 👍 / 👎.
| if (duplicate | ||
| && database.dialect() == Database.Dialect.MYSQL | ||
| && (key == null | ||
| || !key.equals(job.uniqueKey()) | ||
| || !job.uniqueStates().contains(job.state()))) |
There was a problem hiding this comment.
When MySQL's broad ON DUPLICATE KEY fires on a user-added unique constraint, this accepts the existing row whenever its key and state are active, but it never checks whether the incoming job's stateMask covers the incoming state. If the same River key is inactive for the new job, the River functional index could not have caused the conflict, so treating it as a River duplicate silently drops a valid insert. Include the incoming state-bit check before accepting the duplicate.
Useful? React with 👍 / 👎.
|
@codex review Addressed the latest findings in 6979464 and 6d5097c: structural metadata containment, active-state batch uniqueness, and incoming-state validation for MySQL duplicate inserts. Local review also found the same duplicate-state issue in TypeScript, cross-queue index scans in Java/TypeScript/Ruby, and a Java cleanup/completion deadlock; those are fixed too. Validation: full Go tests and lint; metadata regressions on MySQL 8.0 and 8.4; Java verification on both MySQL versions; 20 repeated Java rescue-test runs; all 46 TypeScript MySQL integration tests plus lint/type checks; Ruby MySQL tests, lint, and type checks. The cleanup regression fails with the original query and passes with the fix. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d5097c3f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| FOR UPDATE; | ||
|
|
||
| -- name: JobDeleteBefore :execresult | ||
| DELETE FROM /* TEMPLATE: schema */river_job |
There was a problem hiding this comment.
When a completion transaction holds a low-ID job lock, this ordered DELETE can block while scanning that record even though the running job does not match the cleanup predicate; if the completion batch subsequently needs a row already locked by the cleaner, InnoDB can deadlock and roll back a completion. The Java MySQL cleaner in this diff replaces the same direct-delete shape with a bounded FOR UPDATE SKIP LOCKED selection, but the Go query here and the new TypeScript/Rust cleaners still use the blocking form, so apply the same skip-locked pattern across them.
Useful? React with 👍 / 👎.
| -- name: JobUpdateExec :exec | ||
| UPDATE /* TEMPLATE: schema */river_job | ||
| SET | ||
| metadata = CASE WHEN CAST(sqlc.arg('metadata_do_merge') AS SIGNED) THEN JSON_MERGE_PATCH(metadata, CAST(sqlc.arg('metadata') AS JSON)) ELSE metadata END |
There was a problem hiding this comment.
When a metadata update contains JSON null, such as {"checkpoint":null}, JSON_MERGE_PATCH removes that key instead of retaining it with a null value as River's shallow merge and Postgres jsonb || do. This changes persisted metadata and makes a subsequent containment filter for that null-valued key fail; use per-member JSON_SET semantics in this query and the batch completion query, and cover the null case in the shared driver suite.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
Select and lock cleaner candidates with SKIP LOCKED before deleting by ID across the MySQL implementations. Keep Java cleanup working without a default database. Preserve JSON nulls and replace nested metadata members at the top level. Shared regressions also correct SQLite merge behavior and Postgres database/sql numeric precision. Isolate cancellation tests from startup maintenance locks.
Force primary-key access for bounded deletes and expand cleanup locking regressions to full 1,000-job batches. This prevents the optimizer from switching to table scans as the ID list grows.
|
@codex review Addressed the latest review in bb7368c and 5d68530:
Verification: full Go tests and lint; Java Maven verify on MySQL 8.0 and 8.4 without a default database; TypeScript MySQL integration tests and lint/type checks; Ruby MySQL tests, lint, and type checks; Rust MySQL tests on 8.0 and 8.4 and the full Clippy matrix. Go MySQL conformance tests also pass on both versions. The local re-review has no remaining findings. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Cancelled queries can leave SQLx waiting for an unfinished statement during pool release, blocking pool shutdown. Discard interrupted River-owned connections and reuse them only after successful operations. Preserve SQLx transaction depth for savepoints and cover cancellation, pool shutdown, and connection reuse with regression tests.
|
@codex review Integrated the previously uncommitted Rust MySQL cancellation fix with the latest PR code. I reproduced the bug on the previously reviewed head: cancelling a query blocked during statement preparation left pool.close() waiting for that connection. River-owned MySQL operations now discard interrupted connections and return connections to the pool after successful completion. Transactions continue to use SQLx's transaction manager so savepoints work correctly. Caller-owned transactions retain their existing ownership semantics. Verification: all 18 MySQL tests pass on 8.0 and 8.4, the Rust unit/SQLite/documentation suite and full Clippy matrix pass, and the cancellation, savepoint/reuse, notification shutdown, and worker maintenance tests pass 25 repeated runs (100 test executions). Local re-review found no remaining issues. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@bgentry Thoughts on moving forward with this? I feel a little more motivated now that I have multiple IRL testimonies of MySQL use that make sense. Rationale similar to Polyglot: probably crazy if everything's hand-maintained. Less so given how automated the process is. We'd probably have a similar strategy to SQLite: main first, Pro later. Plus, gives me something to hack on a bit while the Pro polyglot stuff is in limbo. |
2 participants
Here, add a new driver
rivermysqlthat brings MySQL support to River.Similar to SQLite, it's unfortunately not quite as good as the Postgres
driver, but it does the job. MySQL has more facilities than SQLite, but
it's still missing some major niceties like
RETURNING, which for manyqueries requires us to write an implementation using two operations --
one that performs the action, and then another that loads back the
result using returned IDs. It also doesn't have listen/notify.
Luckily, like SQLite, any of this nastiness stays cordoned to the
driver layer and doesn't leak into the mainline River code. We've got
good driver testing and basic tests on clients for each driver in place
so we get reasonable assurance that everything works.
In the before times, I would've been fairly concerned at the additional
maintenance burden that supporting another database would bring, but
with the rise of LLMs I think it's more plausible that we can bring
something like this in without much trouble. Incredibly, I was able to
get almost all of this implemented in just one evening whereby my SQLite
driver took me multiple months.
I don't want to bring MySQL in as a hard dependency, so I've made the
MySQL tests disabled by default. Use
RIVER_MYSQL_TESTS_ENABLED=truetoactivate them. Only one CI matrix case runs MySQL tests so that we don't
have to repeat them to exhaustion on every version of Go and Postgres.
See also:
#158