diff --git a/.serena/memories/backend/storage.md b/.serena/memories/backend/storage.md index cfd16f2976..3827a496ba 100644 --- a/.serena/memories/backend/storage.md +++ b/.serena/memories/backend/storage.md @@ -139,7 +139,7 @@ Since `put-object!` uses backend-specific operations (`impl/resolve-backend` + ` - It has a `kind` column: `1` is a snapshot (it has a label and its data lives in `file_data`) and `2` is a revision (it has no label and it holds the changes). The values are in `app.features.file-changes`. - `kind` is a deliberate reserve, not a leftover: today no query filters by it, the queries still tell a snapshot from a revision by its label (`label IS NOT NULL`), and `kind` takes over when they are rewritten. No constraint ties `kind` and `label` together on purpose: the day `kind` is the source of truth, that constraint would have to go. - Every write of the application to a row of `file_change` moves `updated_at` to the moment of the write (`lock-by!`, `unlock!`, the snapshot rename and delete, `delete-object`, the restore of a file, and the `legacy-db` snapshot write of `fdata`). The actions of the foreign keys (`ON DELETE SET NULL` on `locked_by` and `profile_id`) change the row without touching it; during the partition both tables carry the same keys, so both receive them. The runbook of the partition uses it as the watermark of its delta passes: a write of the application that leaves it behind is lost at the cutover. -- `file_change_legacy` is the way back of the partition: it keeps the rows and the indexes of the old table. A new instance (empty table) does not keep it. On an upgraded instance the table stays until the operator drops it by hand once the rollback window is over (`DROP TABLE file_change_legacy;`, days later, see `operations/2.19.0-file-change-partition.md`); nothing drops it automatically. +- `file_change_legacy` is a frozen copy of the previous layout of the partition: it keeps the rows and the indexes of the old table. A new instance (empty table) does not keep it. On an upgraded instance the table stays until the operator drops it by hand days later (`DROP TABLE file_change_legacy;`, see `operations/2.19.0-file-change-partition.md`); nothing drops it automatically. - A revision carries only the delta of a save (the changes). The features of the file are not repeated on it: nobody read them from there. - A snapshot carries the features and the migrations of the file. - It keeps the `data` column: the `legacy-db` backend is still supported (migration `0157-migrate-legacy-file-data` moved the existing blobs, but the backend keeps reading and writing that column). It has no `data_ref_id` or `data_backend` columns: nothing read them since the file storage refactor. diff --git a/backend/src/app/migrations/sql/0158-partition-file-change.sql b/backend/src/app/migrations/sql/0158-partition-file-change.sql index d9461d40e4..b04f1813c2 100644 --- a/backend/src/app/migrations/sql/0158-partition-file-change.sql +++ b/backend/src/app/migrations/sql/0158-partition-file-change.sql @@ -22,9 +22,9 @@ -- they are rewritten. That is why no constraint ties the two together: the -- day `kind` is the source of truth, the constraint would have to go. -- --- The old table is kept aside as `file_change_legacy`: it is the way back if --- something goes wrong. It is dropped by hand, days later (or right here, --- when it is empty: on a new instance there is nothing to go back to). +-- The old table is kept aside as `file_change_legacy`: a frozen copy of the +-- previous layout. It is dropped by hand, days later (or right here, when it +-- is empty: on a new instance there is nothing to keep). -- -- This migration is not meant to run on a big instance: the copy of the -- rows of the table happens inside the transaction of the migration, and @@ -229,15 +229,15 @@ CREATE INDEX file_change__user_snapshots__idx -- this migration, so it is the worst moment to leave them unanalyzed. ANALYZE file_change; --- 9. The legacy table is kept: it is the way back. It is dropped by hand, --- days after the cutover: +-- 9. The legacy table is kept as a frozen copy of the previous layout. It is +-- dropped by hand, days after the cutover: -- DROP TABLE file_change_legacy; -- -- Its foreign key on `file_id` goes away with the copy: it is `NO -- ACTION`, so while the legacy table has rows, the objects GC cannot -- delete a file (it deletes the rows of the file first, and the legacy --- ones are not its business). The legacy table is frozen, and the --- rollback adds the constraint back with `NOT VALID`. +-- ones are not its business). The legacy table is frozen: nothing writes +-- to it again. ALTER TABLE file_change_legacy DROP CONSTRAINT file_change_file_id_fkey_legacy; diff --git a/backend/src/app/rpc/commands/files_update.clj b/backend/src/app/rpc/commands/files_update.clj index 86e69be7f8..1da45b1c5a 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -344,7 +344,6 @@ :file-id (:id file) :revn (:revn repair) :version (:version file) - :features (into-array (:features file)) :changes (blob/encode (:changes repair))} {::db/return-keys false}) (l/wrn :hint "file auto-repaired" diff --git a/backend/test/backend_tests/migrations_partition_file_change_test.clj b/backend/test/backend_tests/migrations_partition_file_change_test.clj index 7e2dbd6cef..d5cc29ad37 100644 --- a/backend/test/backend_tests/migrations_partition_file_change_test.clj +++ b/backend/test/backend_tests/migrations_partition_file_change_test.clj @@ -110,7 +110,10 @@ :created-by "user" :created-at created-at :updated-at updated-at - :deleted-at deleted-at}) + :deleted-at deleted-at + ;; a row that still carries the dead columns + :data-ref-id (uuid/next) + :data-backend "legacy-db"}) (db/insert! conn :file-change {:id rev-id @@ -174,4 +177,34 @@ ORDER BY con.conname"]) (mapv :name))))) + (t/testing "the dead columns are not carried over" + ;; The old row carried `data_ref_id` and `data_backend`; + ;; the new table has no such columns. + (t/is (= [] + (->> (db/exec! conn ["SELECT attname AS name + FROM pg_attribute + WHERE attrelid = 'file_change'::regclass + AND attnum > 0 AND NOT attisdropped + AND attname IN ('data_ref_id', 'data_backend')"]) + (mapv :name))))) + + (db/exec! conn [(str "DROP SCHEMA " schema " CASCADE")]))))) + +(t/deftest drops-the-legacy-table-when-it-is-empty + ;; On a new instance there is nothing to keep: the migration leaves no + ;; legacy table behind. The relation is asserted on its own, so a missing + ;; table can never pass for a clean one. + (let [schema (str "test_0158_empty_" (subs (str (uuid/next)) 0 8))] + (db/tx-run! th/*pool* + (fn [{:keys [::db/conn]}] + (db/exec! conn [(str "CREATE SCHEMA " schema)]) + (db/exec! conn [(str "SET LOCAL search_path TO " schema)]) + (db/exec! conn [sql:old-schema]) + + (run-migration! conn) + + (t/is (-> (db/exec-one! conn ["SELECT to_regclass('file_change_legacy') IS NULL AS empty"]) + :empty) + "the empty legacy table is dropped") + (db/exec! conn [(str "DROP SCHEMA " schema " CASCADE")]))))) diff --git a/backend/test/backend_tests/rpc_file_snapshot_test.clj b/backend/test/backend_tests/rpc_file_snapshot_test.clj index 0af89b8397..ac417f8ee1 100644 --- a/backend/test/backend_tests/rpc_file_snapshot_test.clj +++ b/backend/test/backend_tests/rpc_file_snapshot_test.clj @@ -861,15 +861,21 @@ :id (:id snapshot)} (= :update-file-snapshot type) (assoc :label "renamed")) - out (th/command! params)] + out (th/command! params) + row (th/db-get :file-change {:id (:id snapshot)} + {::db/remove-deleted false})] (t/is (nil? (:error out))) (t/is (true? (:result out))) - (t/is (pos? (- (inst-ms (:updated-at (th/db-get :file-change - {:id (:id snapshot)} - {::db/remove-deleted false}))) - (inst-ms stale-at))) - "updated_at must be bumped by the mutation")))) + (t/is (pos? (- (inst-ms (:updated-at row)) (inst-ms stale-at))) + "updated_at must be bumped by the mutation") + (t/is (<= (inst-ms (:updated-at row)) (inst-ms (ct/now))) + "updated_at must not be in the future") + (t/is (<= (- (inst-ms (ct/now)) (inst-ms (:updated-at row))) 60000) + "updated_at must be the moment of the mutation") + (when (= type :delete-file-snapshot) + (t/is (< (inst-ms (:updated-at row)) (inst-ms (:deleted-at row))) + "the soft delete must not stamp updated_at with the future deleted_at"))))) (t/testing "the deleted snapshot is marked as deleted" (t/is (some? (:deleted-at (th/db-get :file-change {:id (:id snapshot)} diff --git a/backend/test/backend_tests/rpc_file_update_repair_test.clj b/backend/test/backend_tests/rpc_file_update_repair_test.clj index e681d559ff..532d048ec7 100644 --- a/backend/test/backend_tests/rpc_file_update_repair_test.clj +++ b/backend/test/backend_tests/rpc_file_update_repair_test.clj @@ -318,6 +318,10 @@ (t/is (= 2 (count new))) (t/is (= (inc n) (:revn (first new)))) (t/is (= files-update/repair-session-id (:session-id (first new)))) + ;; the repair is a revision like the save: no features, a changes blob + (t/is (= 2 (:kind (first new)))) + (t/is (nil? (:features (first new)))) + (t/is (some? (:changes (first new)))) (t/is (some? (:deleted-at (first new)))) (t/is (= (:created-at (first new)) (:updated-at (first new)))) (t/is (= (+ n 2) (:revn (second new)))) diff --git a/operations/2.19.0-file-change-partition.md b/operations/2.19.0-file-change-partition.md index 8aba631091..1f0f08498e 100644 --- a/operations/2.19.0-file-change-partition.md +++ b/operations/2.19.0-file-change-partition.md @@ -55,8 +55,8 @@ alone. - The table has 17 columns: `data_ref_id` and `data_backend` are gone (dead since the file storage refactor), `data` is kept (the `legacy-db` backend reads old payloads from it) and `kind` appears (1 = snapshot, 2 = revision). -- The old table is kept as `file_change_legacy`, the way back if something - goes wrong, and dropped days later. +- The old table is kept as `file_change_legacy`, a frozen copy of the + previous layout, and dropped days later. - Almost all of the size of the table is dead space (see sizing), so the new table holds at most its heap + TOAST, and less as the dead rows are not copied. The disk comes back when the legacy table is dropped: the database @@ -332,30 +332,8 @@ ALTER TABLE file_change_new ALTER TABLE file_change_new ADD CONSTRAINT file_change_profile_id_fkey FOREIGN KEY (profile_id) REFERENCES profile(id) ON DELETE SET NULL DEFERRABLE; - --- Secondary indexes: the canonical names, with the _new suffix. There is no --- index for the system snapshots: the old one filtered them by --- `data IS NOT NULL`, and with the payload living in `file_data` the column --- no longer tells snapshots apart. The new one could only filter --- them by `deleted_at IS NULL`, and the system snapshots always carry a --- deletion date in the future, so it would never match a row. -CREATE INDEX file_change_new__deleted_at__idx - ON file_change_new (deleted_at, file_id, id) WHERE deleted_at IS NOT NULL; -CREATE INDEX file_change_new__file_id__revn__idx - ON file_change_new (file_id, revn); -CREATE INDEX file_change_new__locked_by__idx - ON file_change_new (locked_by) WHERE locked_by IS NOT NULL; -CREATE INDEX file_change_new__profile_id__idx - ON file_change_new (profile_id) WHERE profile_id IS NOT NULL; -CREATE INDEX file_change_new__user_snapshots__idx - ON file_change_new (file_id) - WHERE label IS NOT NULL AND created_by = 'user' AND deleted_at IS NULL; ``` -The `deleted_at` index carries `file_id` (the primary key is `(file_id, id)`): -the objects GC needs the `file_id` to lock by pair, and with the index it -reads the rows it has to delete without touching the table. - ```sql SELECT count(*) AS partitions FROM pg_partition_tree('file_change_new') t WHERE t.isleaf; @@ -562,6 +540,33 @@ SELECT count(*) FROM file_change ### 4.1 Before the lock - One last delta pass. +- Build the secondary indexes of the new table. The bulk and the delta passes + only need the primary key (`ON CONFLICT (file_id, id)`), so building them + here keeps five indexes and their WAL out of every batch. The `deleted_at` + index carries `file_id` (the primary key is `(file_id, id)`): the objects GC + needs the `file_id` to lock by pair, and with the index it reads the rows it + has to delete without touching the table. + +```sql +-- Secondary indexes: the canonical names, with the _new suffix. There is no +-- index for the system snapshots: the old one filtered them by +-- `data IS NOT NULL`, and with the payload living in `file_data` the column +-- no longer tells snapshots apart. The new one could only filter +-- them by `deleted_at IS NULL`, and the system snapshots always carry a +-- deletion date in the future, so it would never match a row. +CREATE INDEX file_change_new__deleted_at__idx + ON file_change_new (deleted_at, file_id, id) WHERE deleted_at IS NOT NULL; +CREATE INDEX file_change_new__file_id__revn__idx + ON file_change_new (file_id, revn); +CREATE INDEX file_change_new__locked_by__idx + ON file_change_new (locked_by) WHERE locked_by IS NOT NULL; +CREATE INDEX file_change_new__profile_id__idx + ON file_change_new (profile_id) WHERE profile_id IS NOT NULL; +CREATE INDEX file_change_new__user_snapshots__idx + ON file_change_new (file_id) + WHERE label IS NOT NULL AND created_by = 'user' AND deleted_at IS NULL; +``` + - Prepare and rehearse the cutover script in staging. ### 4.2 The cutover (one transaction, a brief exclusive lock) @@ -619,14 +624,22 @@ ON CONFLICT (file_id, id) DO UPDATE kind = EXCLUDED.kind WHERE file_change_new.updated_at IS DISTINCT FROM EXCLUDED.updated_at; --- Check inside the lock: no row of the old table may be missing. The new --- table can carry extra rows that the GC hard deleted from the source; it --- cleans them up after the cutover. -SELECT count(*) AS missing - FROM file_change o - WHERE NOT EXISTS (SELECT 1 FROM file_change_new n - WHERE n.file_id = o.file_id - AND n.id = o.id); +-- Check inside the lock, and abort if it fails: no row of the old table may +-- be missing in the new one. The new table can carry extra rows that the GC +-- hard deleted from the source; it cleans them up after the cutover. +DO $$ +DECLARE missing bigint; +BEGIN + SELECT count(*) INTO missing + FROM file_change o + WHERE NOT EXISTS (SELECT 1 FROM file_change_new n + WHERE n.file_id = o.file_id + AND n.id = o.id); + + IF missing > 0 THEN + RAISE EXCEPTION 'file_change: % rows of the old table are missing in file_change_new; cutover aborted', missing; + END IF; +END $$; -- The old table frees the canonical names (the same names the migration -- 0158 uses, with the _legacy suffix) @@ -683,8 +696,7 @@ ALTER TABLE file_change_new RENAME TO file_change; -- The foreign key of the legacy table goes away: it is `NO ACTION`, so -- while the legacy table has rows for a file, the objects GC cannot delete -- that file (it deletes the rows of the file first, and the legacy ones are --- not its business). The legacy table is frozen, and the rollback adds the --- constraint back. +-- not its business). The legacy table is frozen: nothing writes to it again. ALTER TABLE file_change_legacy DROP CONSTRAINT file_change_file_id_fkey_legacy; @@ -694,8 +706,8 @@ ANALYZE file_change; ``` - `file_change_legacy` keeps its data and its indexes (with the `_legacy` - suffix): it stays frozen as the way back. Its foreign key on `file_id` is - dropped, so the objects GC can delete the files again. + suffix): a frozen copy of the previous layout. Its foreign key on `file_id` + is dropped, so the objects GC can delete the files again. - The writes to `file_change` during the lock fail: they are seconds-minutes, the affected saves are retried. - The objects GC and the four RPC commands keep working: same table name and @@ -723,26 +735,6 @@ still writing with a prepared statement of the old table: those rows are lost when the legacy table is dropped, so they have to be copied over before that. -### 4.3 Rollback - -- **Before the `COMMIT`**: `ROLLBACK` and that is it. -- **After the cutover**: the new writes go to the partitioned table. To revert, - rename the tables back and copy the delta the other way - (`INSERT INTO file_change_legacy ... WHERE updated_at > :cutover ON CONFLICT - (id) DO UPDATE`, carrying `data` too). The foreign key on `file_id` was dropped at the cutover, - so it goes back with `NOT VALID` (the existing rows are not checked) and it - is validated later: - -```sql -ALTER TABLE file_change_legacy - ADD CONSTRAINT file_change_file_id_fkey - FOREIGN KEY (file_id) REFERENCES file(id) DEFERRABLE NOT VALID; -``` - - Reasonable window: only if there is a serious defect in the first hours. - After that, treat any problem the normal way (the partitioned table is the - good state). - --- ## Phase 5 — After the cutover @@ -902,7 +894,7 @@ size of the old table (see sizing). | Hard delete of the GC between passes | The row is left with `deleted_at` in the new table; the GC cleans it up later | | Orphan rows if a `file` is deleted halfway | The foreign key on `file_id` is `NO ACTION`: the GC deletes the rows of the file before the file, so a chunk that fails leaves the file in the trash and the next run deletes it | | The files in the trash cannot be deleted while the legacy table has rows for them | The foreign key of the legacy table is dropped at the cutover; until then the files wait in the trash | -| Saves failing during the lock of the cutover | Freeze of minutes in a low traffic window; manual retries; the rollback is documented | +| Saves failing during the lock of the cutover | Freeze of minutes in a low traffic window; manual retries | | Planner regression after the cutover | Immediate `ANALYZE` + the pruning `EXPLAIN`; the pool already sets the GUC | | Clock drift between the application and the database | The delta window starts an hour before the bulk; the margin absorbs the drift |