Conversation
cb0f2d6 to
d6d4385
Compare
| unique_states | ||
| FROM /* TEMPLATE: schema */river_job_old; | ||
|
|
||
| DROP TABLE /* TEMPLATE: schema */river_job_old; |
There was a problem hiding this comment.
Dropping river_job_old also drops Pro-owned columns, indexes, and triggers if this database already installed Pro on main 007. The new Pro prerequisite runs only in migration 001, so it cannot protect a database that has already passed that migration.
Upgrade scenario and suggested fix
Pro's migrations add partition_key, workflow_id, and workflow_task to river_job, along with indexes and synchronization triggers. This rebuild copies only OSS columns into the new table; Pro's columns are omitted, while its indexes and triggers stay attached to the renamed table and disappear here. The Pro migration records remain, so rerunning Pro migrations does not restore them. The 008 down migration has the same exposure after Pro has been installed.
If existing Pro SQLite databases are intentionally unsupported, please make this migration fail before the rebuild when it finds an installed Pro schema, and document the reset/upgrade path. If they are supported, the two migration lines need a coordinated upgrade that preserves or reconstructs Pro's data and schema in both allowed directions. A regression test should start at main 007 with Pro installed and populated, then migrate main to 008 and exercise Pro operations.
There was a problem hiding this comment.
Yeah, see PR description but I understand this definitely isn't ideal. Unfortunately given SQLite's lack of serious ALTER TABLE, rebuilding the pro schema from here becomes very complex, and I was trying not to engage in that right now even though we'll probably have to tackle it later.
So instead, we kind of make an assumption that Pro SQLite support is still new enough that our Pro users won't actually be using it yet. We also left it unannounced so far, which should help.
I've modified this slightly to raise an error in case a Pro schema is detected. That way at least we won't be dropping anything silently.
| ALTER TABLE /* TEMPLATE: schema */river_job RENAME TO river_job_old; | ||
|
|
||
| CREATE TABLE /* TEMPLATE: schema */river_job ( | ||
| id integer PRIMARY KEY AUTOINCREMENT, |
There was a problem hiding this comment.
AUTOINCREMENT starts tracking IDs from the rows copied into the new river_job, so an ID that exists only in Pro's dead letter table can still be handed out again after this migration. That leaves the original dead letter conflict possible for an upgraded database.
Example and suggested fix
Suppose the highest live job ID is 41 and river_job_dead_letter contains an earlier job with ID 42. Copying the live rows initializes sqlite_sequence to 41; the next job gets 42, and moving it to dead letter fails on the existing row's primary key. I reproduced this allocation and conflict in SQLite. An empty live job table has the same problem for every archived ID.
If existing Pro SQLite databases are supported, their upgrade should advance the job sequence past the maximum ID in both live and dead letter tables, in the same transaction, and test the subsequent dead letter move. For OSS-only databases, IDs that were issued and then deleted without any surviving record cannot be recovered; the migration can only guarantee no reuse going forward from the highest ID it can observe.
There was a problem hiding this comment.
See above — existing Pro SQLite isn't supported.
Without the `AUTOINCREMENT` keyword in SQLite, it'll pick new primary keys based off the largest ID in the table. There's a potential class of bug currently wherein if the latest row is moved out of a table, it may clash with another added row that's been added since and acquired the same ID. Here, introduce migration 008 which adds `AUTOINCREMENT` to SQLite tables. Unfortunately SQLite doesn't allow DDL in place, so we have some pretty ugly SQL here that redefines `river_job` and moves data over. I looked into whether any other tables might need `AUTOINCREMENT`, but the answer appears to be "no". `river_notification` already uses it, and the other tables all use non-automatic primary keys. SQLite for Pro is brand new and not yet announced, so taking the easier route here and not accounting for that in the schema. It's not great, but greatly simplifies things.
d6d4385 to
52c2188
Compare
| -- from being reused. | ||
| SELECT 1; |
|
|
||
| ## [Unreleased] | ||
|
|
||
| ⚠️ This release contains a new database migration, version 8, that only affects SQLite. It rebuilds `river_job` with `AUTOINCREMENT` to prevent automatically generated job IDs from being reused after deletion. The migration is a no-op for PostgreSQL. On SQLite, both migration directions refuse to run if River Pro schema is already installed, to avoid discarding its additions to `river_job`. |
There was a problem hiding this comment.
Reminder to tweak this if you end up merging other migration fixes as part of this release.
2 participants
Without the
AUTOINCREMENTkeyword in SQLite, it'll pick new primarykeys based off the largest ID in the table. There's a potential class of
bug currently wherein if the latest row is moved out of a table, it may
clash with another added row that's been added since and acquired the
same ID.
Here, introduce migration 008 which adds
AUTOINCREMENTto SQLite tables.Unfortunately SQLite doesn't allow DDL in place, so we have some pretty
ugly SQL here that redefines
river_joband moves data over.I looked into whether any other tables might need
AUTOINCREMENT, butthe answer appears to be "no".
river_notificationalready uses it, andthe other tables all use non-automatic primary keys.
SQLite for Pro is brand new and not yet announced, so taking the easier
route here and not accounting for that in the schema. It's not great,
but greatly simplifies things.