|
| 1 | +--- |
| 2 | +"@objectstack/driver-sql": patch |
| 3 | +--- |
| 4 | + |
| 5 | +fix(driver-sql): `bulkCreate` and `upsert` re-seed a stale autonumber counter instead of burning the whole batch (#6943) |
| 6 | + |
| 7 | +#5495 taught `create()` to re-seed a stale autonumber counter and retry instead |
| 8 | +of burning one number per failed insert. `bulkCreate()` and `upsert()` call the |
| 9 | +same `fillAutoNumberFields` and did not get that fix. They are not, however, the |
| 10 | +same defect as each other — measured on `main` @ `c8ff269`, on a fresh database |
| 11 | +with seeded rows above the counter (the one-time-storm repro constraint #5495 |
| 12 | +established): |
| 13 | + |
| 14 | +**`upsert` is `create()`'s old shape exactly.** Single row, so a stale counter |
| 15 | +costs it one burned number per call: `last_value` walked 1 → 2 → 3 across two |
| 16 | +refused upserts. Its `ON CONFLICT (mergeKeys) DO UPDATE` absorbs a conflict on |
| 17 | +the merge key only; the tenanted autonumber lives under a *different* unique |
| 18 | +index, so that violation is still raised and still reaches the caller. |
| 19 | + |
| 20 | +**`bulkCreate` is worse.** Each row reserves its number in its own committed |
| 21 | +transaction and the batch then goes in as ONE insert, so a single colliding row |
| 22 | +burns *every* number the batch reserved and fails the whole request: |
| 23 | + |
| 24 | +| 3-row `bulkCreate`, counter at 10, rows 11–39 already present | before | after | |
| 25 | +|:---|:---|:---| |
| 26 | +| caller-visible failures | both calls threw | **0** | |
| 27 | +| rows written | **0** | 3 | |
| 28 | +| `last_value` | 10 → 13, then 13 → 16 | 10 → 42, by one re-seed | |
| 29 | + |
| 30 | +And it is the worst path to leave without recovery: framework#2678 made |
| 31 | +`bulkCreate` the common case for seed/import, and seed/import is exactly what |
| 32 | +*creates* the staleness — an `isSystem` replay or a `preserveAudit` import keeps |
| 33 | +its explicit numbers and never enters `fillAutoNumberFields` (#5495/#5503). |
| 34 | + |
| 35 | +Both paths now reuse #5495's machinery unchanged — `collidingAutoNumberReservations` |
| 36 | +for the three-state routing, `autoNumberValueExists` for the data-based |
| 37 | +discriminator (the conflicting column is never determinable for a tenanted |
| 38 | +autonumber), and the forward-only `resyncSequenceToDataMax`. A collision that is |
| 39 | +not provably this counter's is still rethrown untouched, so a duplicate on a |
| 40 | +value the caller supplied still reaches them as its own error. |
| 41 | + |
| 42 | +**Batch semantics are unchanged, and that is a measurement rather than a |
| 43 | +choice.** `insert(rows[])` is a single statement, so the batch was already |
| 44 | +all-or-nothing — the failed batch above left the table exactly as it found it. |
| 45 | +Re-issuing and retrying the whole batch therefore preserves the existing |
| 46 | +contract: no partial success is introduced, no transaction is opened, and no |
| 47 | +"does a failed row roll back its siblings" question arises, because siblings |
| 48 | +already fail together. Per-row retry inside the batch was rejected for the |
| 49 | +opposite reason — it would have had to split the one statement into N and invent |
| 50 | +partial success where none existed. |
| 51 | + |
| 52 | +One thing the batch may not borrow from `create()`: `create()` keeps a |
| 53 | +reservation that did not collide, to avoid burning a second number. A batch |
| 54 | +cannot. One that straddles the seeded range has its low rows collide and its |
| 55 | +high rows not, and re-issuing only the collided ones would hand them numbers |
| 56 | +*above* the kept ones — an intra-batch duplicate the driver would have |
| 57 | +manufactured itself. Re-issue is therefore per counter: every row drawn from a |
| 58 | +counter that went stale is re-issued, and counters that did not go stale keep |
| 59 | +their values, so a co-tenant's rows in the same batch are undisturbed. |
| 60 | + |
| 61 | +As with #5495, retrying is confined to the no-caller-transaction case. Inside a |
| 62 | +caller's transaction the sequence `UPDATE` rolls back with the refused `INSERT`, |
| 63 | +so nothing is burned and there is nothing to repair (measured on both paths), and |
| 64 | +on Postgres a constraint failure aborts the transaction outright. The caller owns |
| 65 | +that retry. |
| 66 | + |
| 67 | +`TursoDriver` (local/replica) and `SqliteWasmDriver` inherit both fixes, each |
| 68 | +pinned by its own test rather than assumed from the base class — Turso |
| 69 | +*overrides* `bulkCreate`/`upsert` to route remote traffic away, so inheritance |
| 70 | +there is a routing fact, not a class fact. Turso's remote transport builds its |
| 71 | +own INSERT and generates no autonumber at all, so it neither has this defect nor |
| 72 | +receives this fix (that gap is #6944). |
0 commit comments