github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-11-08 11:04:44 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Address the review of the file_change partition

Browse Source 
The production runbook promised a rollback it did not have: the section
was a sketch, and its statements could write to the table about to be
dropped. The rollback is removed, and so is every comment that described
it. `file_change_legacy` stays as a frozen copy of the previous layout.

The cutover check now aborts the transaction when a row of the old table
is missing in the new one, instead of leaving the decision to the
operator.

The five secondary indexes of the new table are built after the last
delta pass: the bulk and the delta passes only need the primary key, so
the batches no longer maintain them.

The copy of 0158 is now replayed in a test against the pre-0158 shape
with a labelled and an unlabelled row, plus an empty instance that must
leave no legacy table behind. The updated_at assertion of the snapshot
mutations is bounded by the test clock, and the repair revision stops
writing the `features` of the file, like the normal save.

AI-assisted-by: deepseek-v4.1-flash
This commit is contained in:
Andrey Antukh 2026-11-05 14:03:30 +00:00
parent 67643340a2
commit 8affb410ff
7 changed files with 107 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.serena/memories/backend/storage.md
View File

@ -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.

14
backend/src/app/migrations/sql/0158-partition-file-change.sql
View File

@ -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;

1
backend/src/app/rpc/commands/files_update.clj
View File

@ -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"

35
backend/test/backend_tests/migrations_partition_file_change_test.clj
View File

@ -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")])))))

18
backend/test/backend_tests/rpc_file_snapshot_test.clj
View File

@ -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)}

4
backend/test/backend_tests/rpc_file_update_repair_test.clj
View File

@ -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))))

106
operations/2.19.0-file-change-partition.md
View File

@ -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 |