Skip to content

Commit 431d2fb

Browse files
os-zhuangclaude
andauthored
fix(driver-sql): collect the hash shadow a retired index leaves behind (#13084)
* fix(driver-sql): collect the hash shadow a retired index leaves behind Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry * test(driver-sql): pin the orphan hash-shadow cleanup, plus changeset Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e9c1055 commit 431d2fb

4 files changed

Lines changed: 743 additions & 4 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/driver-sql': patch
3+
---
4+
5+
Retiring a shadow-carried UNIQUE index no longer leaves its generated column behind forever (#13056).
6+
7+
`isHashShadowColumn`'s docblock is why the orphan-COLUMN drift pass skips a #11627 hash shadow, and it stated what happens instead: the column "is then cleaned up by the index's own removal path, not by a blind column drop". There was no such path. `dropIndexIfExists` issues one statement family — `ALTER TABLE .. DROP CONSTRAINT`, `DROP INDEX IF EXISTS`, `ALTER TABLE .. DROP INDEX` — and never touches a column. So when metadata stopped declaring the index, `diffManagedIndexes` reported it as an orphan, `os migrate apply --allow-destructive` dropped it, and the `VARBINARY(32)` STORED generated column survived keyed by nothing, while the orphan-column pass declined to report it forever, exactly as designed. A STORED generated column is recomputed and written on every INSERT and on every UPDATE touching its source columns, so a table accumulating retired declarations paid for them permanently and silently.
8+
9+
The `drop_index` op now collects that column after dropping the index. Ownership is established first, never assumed — in the shape of #13015's `foreign` guard, a column the driver has not proved is its own is left in place and named in a warning rather than dropped: a column of that name that is **not generated** may hold user data, and a column some **other index still keys** is not this orphan (that second read is what makes "index first, then column" a checked precondition rather than an ordering comment). An unreadable catalog degrades to leaving the column alone.
10+
11+
**Why the cleanup hangs off the op and not off `dropIndexIfExists`,** which has two other callers. The discriminator is not *which caller* but *is this index name coming back*, and only the op knows. `recreate_index` drops in order to re-create under the same name, and its shadow must survive: #13015's `reusable` branch re-keys the survivor in place instead of rebuilding the table around a regenerated STORED column, and a cleanup in the shared helper would destroy exactly that survivor on every rebuild. `replace_unique_index`'s legacy-name drop cannot reach a shadow at all — #13015 already excludes `isHashShadowCarrier` from legacy detection, in `diffManagedIndexes`, saying it does so *because* that op drops the legacy name. Both are pinned in the negative direction, since they are what a later move of the drop into the shared helper would break and nothing else would notice.
12+
13+
**Why `patch` and not `minor`.** Nothing new is authorable, no export is added (the collector is `protected`), and no input that was accepted is now rejected or vice versa. What an operator will observe that they did not before is a `DROP COLUMN` in the applied set of a migration they had already opted into: the `drop_index` op was already `category: 'destructive'` and already required `--allow-destructive`, so the opt-in is unchanged — the difference is that it now finishes the job it named instead of leaving half of it on the table. A `drop_index` that finds the index already gone is now reported as *applied* rather than skipped when it collects the leftover column, because the apply did rewrite the table.

packages/drivers/driver-sql/src/schema-drift.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -382,8 +382,21 @@ export const HASH_SHADOW_SUFFIX = '__hash';
382382
* Matched by SUFFIX rather than by a registry of known names, deliberately: the
383383
* differ runs against a database whose metadata it is comparing to, and a
384384
* shadow whose declared index has since been removed must still be recognised
385-
* as driver-owned (it is then cleaned up by the index's own removal path, not
386-
* by a blind column drop).
385+
* as driver-owned (it is cleaned up by the index's own removal path, not by a
386+
* blind column drop).
387+
*
388+
* ⚠️ That last clause was an ASSERTION about a path that did not exist, and it
389+
* is the load-bearing half of this docblock: it is the whole reason this pass
390+
* may decline to report the column. #13056 built the path it names —
391+
* `SqlDriver.dropOrphanedHashShadowColumn`, run by the `drop_index` op after
392+
* the index goes — because until then `dropIndexIfExists` issued one statement
393+
* family and never touched a column. The shadow of a retired declaration
394+
* therefore outlived it forever: a `VARBINARY(32)` STORED generated column,
395+
* recomputed on every INSERT and on every UPDATE touching its sources, keyed by
396+
* nothing and reported by no pass. Skipping the column here is only correct
397+
* while some other path really does collect it — so if that method is ever
398+
* removed or its call site moved, this `continue` becomes a leak again and the
399+
* skip must go with it.
387400
*/
388401
export function isHashShadowColumn(name: string): boolean {
389402
return name.endsWith(HASH_SHADOW_SUFFIX);

0 commit comments

Comments
 (0)