diff --git a/.serena/memories/backend/http-storage-filedata-subtleties.md b/.serena/memories/backend/http-storage-filedata-subtleties.md index 188ece7277..fb735b6c56 100644 --- a/.serena/memories/backend/http-storage-filedata-subtleties.md +++ b/.serena/memories/backend/http-storage-filedata-subtleties.md @@ -25,4 +25,7 @@ - File data backends are `legacy-db`, `db`, and `storage`. The storage backend keeps encoded file data in storage bucket `file-data`; the DB row stores metadata with `storage-ref-id` and nil data. - `fdata/upsert!` touches any storage object referenced by incoming metadata before storing the new row/blob. - Pointer-map fragments are persisted separately as type `fragment`, and only modified pointer maps are written. +- File snapshots (versions) keep their metadata row in `file_change` and their payload in `file_data` (`type = 'snapshot'`). With the `db` backend `file_change.data` stays NULL: that column is only written by the `legacy-db` backend, plus the rows written before the 2025-07 file storage refactor. +- Tell a snapshot apart by `label IS NOT NULL` (with `created_by` / `deleted_at` as needed), never by `file_change.data IS NOT NULL`: the label holds with every file data backend, and the payload column stopped meaning "this is a snapshot" when the payload moved out. Queries that identify versions must use the label. +- The version quota counts of `app.rpc.quotes` (`sql:get-snapshots-per-file`, `sql:get-snapshots-per-team`) are served by the partial index `file_change__user_snapshots__idx`, whose predicate mirrors their where clause; changing one without the other drops the index only scan and the count walks the whole table again. - `fdata/realize` combines pointer realization and object-map realization. Use it before operations that need complete in-memory file data instead of pointer placeholders. diff --git a/.serena/memories/prod-infra/core.md b/.serena/memories/prod-infra/core.md index e90f353b7e..5dfaeebed7 100644 --- a/.serena/memories/prod-infra/core.md +++ b/.serena/memories/prod-infra/core.md @@ -39,6 +39,7 @@ Production runs the deployed toolbox, not the devenv one: - **Start from the ids, not from an inventory.** The ids come from the error or the logs. Unbounded queries over tables with tens of millions of rows (`storage_object`, `file_media_object`) do not finish in a reasonable time; bounded to an id list they are index lookups. - **One query per reference column.** Two columns in one `IN (col_a, col_b)` lose the index and scan the whole table; split them. - The read-only replica cuts long queries: see `mem:prod-infra/grafana`. +- A migration that builds an index runs inside the migration transaction, so it cannot use `CONCURRENTLY`, and a plain `CREATE INDEX` locks the table against writes for the whole build. On a large table (file saves write to `file_change` on every save) build the index out of band, before deploying, with `CREATE INDEX CONCURRENTLY` on its own (never inside a transaction block); the shipped migration uses `IF NOT EXISTS`, finds it and does nothing. A concurrent build interrupted midway leaves the index invalid: drop it and retry (check `pg_indexes` and `pg_index.indisvalid`). ## See also diff --git a/backend/src/app/migrations.clj b/backend/src/app/migrations.clj index a3c4647a33..861629bb54 100644 --- a/backend/src/app/migrations.clj +++ b/backend/src/app/migrations.clj @@ -505,7 +505,10 @@ :fn (mg/resource "app/migrations/sql/0153-del-orphan-profile-rels-after-team-leave.sql")} {:name "0154-del-legacy-file-object-thumbnail-table" - :fn (mg/resource "app/migrations/sql/0154-del-legacy-file-object-thumbnail-table.sql")}]) + :fn (mg/resource "app/migrations/sql/0154-del-legacy-file-object-thumbnail-table.sql")} + + {:name "0155-add-idx-to-file-change" + :fn (mg/resource "app/migrations/sql/0155-add-idx-to-file-change.sql")}]) (defn apply-migrations! [pool name migrations] diff --git a/backend/src/app/migrations/sql/0155-add-idx-to-file-change.sql b/backend/src/app/migrations/sql/0155-add-idx-to-file-change.sql new file mode 100644 index 0000000000..28357cc75c --- /dev/null +++ b/backend/src/app/migrations/sql/0155-add-idx-to-file-change.sql @@ -0,0 +1,38 @@ +-- Index for the version (snapshot) quota checks of app.rpc.quotes: +-- sql:get-snapshots-per-file and sql:get-snapshots-per-team count the live +-- versions created by a user, and this partial index answers both counts +-- with an index only scan, without walking the whole file_change table. +-- +-- The predicate must stay identical to the queries: the planner only uses a +-- partial index when the query where clause implies the index predicate. +-- Versions are told apart by label (every snapshot has one); file_change.data +-- is a legacy column, only written by the legacy-db file data backend. + +CREATE INDEX IF NOT EXISTS file_change__user_snapshots__idx + ON file_change (file_id) + WHERE label IS NOT NULL + AND created_by = 'user' + AND deleted_at IS NULL; + +-- NOTE: on instances where file_change is large, run the statement above +-- out of band, before deploying, with CONCURRENTLY instead: this migration +-- runs inside a transaction, and a plain CREATE INDEX locks the table +-- against writes for the whole build (in Penpot every file save writes a row +-- here), while CONCURRENTLY does not. Run it on its own, not inside a +-- transaction block: +-- +-- CREATE INDEX CONCURRENTLY IF NOT EXISTS file_change__user_snapshots__idx +-- ON file_change (file_id) +-- WHERE label IS NOT NULL +-- AND created_by = 'user' +-- AND deleted_at IS NULL; +-- +-- A CONCURRENTLY build interrupted midway leaves the index INVALID: drop it +-- and retry. Verify with: +-- +-- SELECT indexdef FROM pg_indexes +-- WHERE indexname = 'file_change__user_snapshots__idx'; +-- SELECT indisvalid FROM pg_index +-- WHERE indexrelid = 'file_change__user_snapshots__idx'::regclass; +-- +-- Once the index exists, this migration finds it and does nothing. diff --git a/backend/src/app/rpc/quotes.clj b/backend/src/app/rpc/quotes.clj index e391f0b0ac..9f2e67c8ec 100644 --- a/backend/src/app/rpc/quotes.clj +++ b/backend/src/app/rpc/quotes.clj @@ -427,7 +427,7 @@ WHERE fc.file_id = ? AND fc.created_by = 'user' AND fc.deleted_at IS NULL - AND fc.data IS NOT NULL") + AND fc.label IS NOT NULL") (defmethod check-quote ::snapshots-per-file [{:keys [::profile-id ::file-id ::team-id ::project-id ::target] :as quote}] @@ -460,7 +460,7 @@ WHERE p.team_id = ? AND fc.created_by = 'user' AND fc.deleted_at IS NULL - AND fc.data IS NOT NULL") + AND fc.label IS NOT NULL") (defmethod check-quote ::snapshots-per-team [{:keys [::profile-id ::team-id ::target] :as quote}] diff --git a/backend/test/backend_tests/rpc_quotes_test.clj b/backend/test/backend_tests/rpc_quotes_test.clj index b279ff96d3..b0740f1b22 100644 --- a/backend/test/backend_tests/rpc_quotes_test.clj +++ b/backend/test/backend_tests/rpc_quotes_test.clj @@ -472,3 +472,97 @@ (t/is (= :restriction (th/ex-type error))) (t/is (= :max-quote-reached (th/ex-code error))) (t/is (= "media-storage-bytes-per-team" (:target (ex-data error)))))))) + +;; --- SNAPSHOTS (VERSIONS) QUOTES + +(defn- check-snapshot-ok + "Save a version of the file and check that it was created." + [profile file label] + (let [out (th/command! {::th/type :create-file-snapshot + ::rpc/profile-id (:id profile) + :file-id (:id file) + :label label})] + ;; (th/print-result! out) + (t/is (nil? (:error out))) + (t/is (= label (get-in out [:result :label]))) + out)) + +(defn- check-snapshot-ko + "Try to save a version of the file and check that the quota refused it." + [profile file label target] + (let [out (th/command! {::th/type :create-file-snapshot + ::rpc/profile-id (:id profile) + :file-id (:id file) + :label label}) + error (:error out)] + ;; (th/print-result! out) + (t/is (not (th/success? out))) + (t/is (= :restriction (th/ex-type error))) + (t/is (= :max-quote-reached (th/ex-code error))) + (t/is (= target (:target (ex-data error)))) + out)) + +(t/deftest snapshots-per-file-quote + (with-mocks [mock {:target 'app.config/get + :return (th/config-get-mock + {:quotes-snapshots-per-file 2 + :quotes-snapshots-per-team 1000})}] + + (let [profile (th/create-profile* 1) + project (th/create-project* 1 {:profile-id (:id profile) + :team-id (:default-team-id profile)}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:id project) + :is-shared false})] + + (check-snapshot-ok profile file "version1") + (check-snapshot-ok profile file "version2") + (check-snapshot-ko profile file "version3" "snapshots-per-file")))) + +(t/deftest snapshots-per-team-quote + (with-mocks [mock {:target 'app.config/get + :return (th/config-get-mock + {:quotes-snapshots-per-file 1000 + :quotes-snapshots-per-team 3})}] + + (let [profile (th/create-profile* 1) + project (th/create-project* 1 {:profile-id (:id profile) + :team-id (:default-team-id profile)}) + file-1 (th/create-file* 1 {:profile-id (:id profile) + :project-id (:id project) + :is-shared false}) + file-2 (th/create-file* 2 {:profile-id (:id profile) + :project-id (:id project) + :is-shared false})] + + ;; the quota adds up the versions of every file of the team + (check-snapshot-ok profile file-1 "version1") + (check-snapshot-ok profile file-2 "version2") + (check-snapshot-ok profile file-1 "version3") + (check-snapshot-ko profile file-2 "version4" "snapshots-per-team")))) + +(t/deftest snapshots-quote-freed-on-delete + (with-mocks [mock {:target 'app.config/get + :return (th/config-get-mock + {:quotes-snapshots-per-file 2 + :quotes-snapshots-per-team 1000})}] + + (let [profile (th/create-profile* 1) + project (th/create-project* 1 {:profile-id (:id profile) + :team-id (:default-team-id profile)}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:id project) + :is-shared false}) + snapshot (:result (check-snapshot-ok profile file "version1"))] + + (check-snapshot-ok profile file "version2") + (check-snapshot-ko profile file "version3" "snapshots-per-file") + + ;; deleting a version takes it out of the count right away + (let [out (th/command! {::th/type :delete-file-snapshot + ::rpc/profile-id (:id profile) + :id (:id snapshot)})] + (t/is (nil? (:error out))) + (t/is (true? (:result out))) + + (check-snapshot-ok profile file "version4")))))