diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index aa0840b3cd..3f08db34e5 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -430,11 +430,7 @@ ;; A row's `data_version` is the version its changes were ;; written at; the document migrates to it before the row ;; applies, and in the steady state that check is one set - ;; lookup. A NULL stamp marks a row written before the column - ;; existed: it replays at the version the document holds, which - ;; is the base's version, because such rows precede every - ;; stamped row of their log. That is how the derive replayed - ;; every row before the column existed. + ;; lookup. [doc depth] (reduce (fn [[doc depth] {:keys [changes data-version]}] (let [changes (blob/decode changes) diff --git a/backend/src/app/migrations.clj b/backend/src/app/migrations.clj index ce7be5ca10..4dc4cee0c3 100644 --- a/backend/src/app/migrations.clj +++ b/backend/src/app/migrations.clj @@ -526,10 +526,7 @@ :fn (mg/resource "app/migrations/sql/0156-add-file-pull-request-tables.sql")} {:name "0157-add-file-branch-change-table" - :fn (mg/resource "app/migrations/sql/0157-add-file-branch-change-table.sql")} - - {:name "0158-add-file-branch-change-data-version" - :fn (mg/resource "app/migrations/sql/0158-add-file-branch-change-data-version.sql")}]) + :fn (mg/resource "app/migrations/sql/0157-add-file-branch-change-table.sql")}]) (defn apply-migrations! [pool name migrations] diff --git a/backend/src/app/migrations/sql/0155-add-file-branch-table.sql b/backend/src/app/migrations/sql/0155-add-file-branch-table.sql index 43430eab42..6f54d3c9f6 100644 --- a/backend/src/app/migrations/sql/0155-add-file-branch-table.sql +++ b/backend/src/app/migrations/sql/0155-add-file-branch-table.sql @@ -1,13 +1,7 @@ --- Idempotent on purpose: this migration was originally registered as --- "0150-add-file-branch-table" / "0152-add-file-branch-table" during --- development, so instances that already applied it under an old name --- re-apply this one safely (and pick up the later-added --- base_branch_revn column). - ALTER TABLE file - ADD COLUMN IF NOT EXISTS is_branch boolean NOT NULL DEFAULT false; + ADD COLUMN is_branch boolean NOT NULL DEFAULT false; -CREATE TABLE IF NOT EXISTS file_branch ( +CREATE TABLE file_branch ( id uuid PRIMARY KEY, created_at timestamptz NOT NULL DEFAULT clock_timestamp(), @@ -40,32 +34,11 @@ CREATE TABLE IF NOT EXISTS file_branch ( CHECK (branch_file_id != source_file_id) ); --- upgrade path from the 0150 development shape -ALTER TABLE file_branch - ADD COLUMN IF NOT EXISTS base_branch_revn bigint NOT NULL DEFAULT 0; - --- backfill: at creation both counters coincide, so base_revn is the best --- available approximation for rows created before the column existed. --- The rows this is for kept the copy model, where the branch file --- persisted its own data payload; an op-log branch stores no payload and --- legitimately starts its own counter at 0, so the payload's existence --- is what tells the two apart, and the statement changes nothing on a --- re-run (a renamed re-registration re-applies the whole file). -UPDATE file_branch fb - SET base_branch_revn = fb.base_revn - FROM file f - WHERE fb.branch_file_id = f.id - AND fb.base_branch_revn = 0 - AND EXISTS (SELECT 1 - FROM file_data fd - WHERE fd.file_id = f.id - AND fd.deleted_at IS NULL); - -CREATE INDEX IF NOT EXISTS file_branch__source_file_id__idx +CREATE INDEX file_branch__source_file_id__idx ON file_branch(source_file_id) WHERE deleted_at IS NULL; -CREATE UNIQUE INDEX IF NOT EXISTS file_branch__branch_file_id__idx +CREATE UNIQUE INDEX file_branch__branch_file_id__idx ON file_branch(branch_file_id); -CREATE INDEX IF NOT EXISTS file_branch__base_snapshot_id__idx +CREATE INDEX file_branch__base_snapshot_id__idx ON file_branch(base_snapshot_id) WHERE base_snapshot_id IS NOT NULL; diff --git a/backend/src/app/migrations/sql/0156-add-file-pull-request-tables.sql b/backend/src/app/migrations/sql/0156-add-file-pull-request-tables.sql index a0a5f01095..e7163d5658 100644 --- a/backend/src/app/migrations/sql/0156-add-file-pull-request-tables.sql +++ b/backend/src/app/migrations/sql/0156-add-file-pull-request-tables.sql @@ -1,4 +1,4 @@ -CREATE TABLE IF NOT EXISTS file_pull_request ( +CREATE TABLE file_pull_request ( id uuid PRIMARY KEY, created_at timestamptz NOT NULL DEFAULT clock_timestamp(), @@ -33,17 +33,17 @@ CREATE TABLE IF NOT EXISTS file_pull_request ( ); -- at most one live pull request per branch -CREATE UNIQUE INDEX IF NOT EXISTS file_pull_request__file_branch_id__open__idx +CREATE UNIQUE INDEX file_pull_request__file_branch_id__open__idx ON file_pull_request(file_branch_id) WHERE status = 'open' AND deleted_at IS NULL; -CREATE INDEX IF NOT EXISTS file_pull_request__target_file_id__idx +CREATE INDEX file_pull_request__target_file_id__idx ON file_pull_request(target_file_id) WHERE deleted_at IS NULL; -CREATE INDEX IF NOT EXISTS file_pull_request__review_snapshot_id__idx +CREATE INDEX file_pull_request__review_snapshot_id__idx ON file_pull_request(review_snapshot_id) WHERE review_snapshot_id IS NOT NULL; -CREATE TABLE IF NOT EXISTS file_pull_request_review ( +CREATE TABLE file_pull_request_review ( pull_request_id uuid NOT NULL REFERENCES file_pull_request(id) ON DELETE CASCADE DEFERRABLE, profile_id uuid NOT NULL REFERENCES profile(id) ON DELETE CASCADE DEFERRABLE, @@ -61,5 +61,5 @@ CREATE TABLE IF NOT EXISTS file_pull_request_review ( PRIMARY KEY (pull_request_id, profile_id) ); -CREATE INDEX IF NOT EXISTS file_pull_request_review__profile_id__idx +CREATE INDEX file_pull_request_review__profile_id__idx ON file_pull_request_review(profile_id); diff --git a/backend/src/app/migrations/sql/0157-add-file-branch-change-table.sql b/backend/src/app/migrations/sql/0157-add-file-branch-change-table.sql index ed44a3d7a4..5913db608e 100644 --- a/backend/src/app/migrations/sql/0157-add-file-branch-change-table.sql +++ b/backend/src/app/migrations/sql/0157-add-file-branch-change-table.sql @@ -14,8 +14,19 @@ -- dropped (materialize-file-branch), or until the objects GC hard-deletes -- the branch file row and the foreign keys cascade; a logical branch -- deletion leaves them in place. +-- +-- `data_version` is the data version each row was written at: the name +-- of the last file data migration +-- (`app.common.files.migrations/available-migrations`) of the backend +-- that applied the row's changes. A data version is a prefix of that +-- list, so its last name identifies it. The branch derive +-- (`app.binfile.common/branch-file-data`) replays the log in segments: +-- it migrates the document to a row's version before it applies the +-- row, the point where an ordinary file migrates across an upgrade. The +-- merge base needs no column of its own: its snapshot row +-- (`file_change.migrations`) records the migrations its data holds. -CREATE TABLE IF NOT EXISTS file_branch_change ( +CREATE TABLE file_branch_change ( id uuid PRIMARY KEY, branch_id uuid NOT NULL REFERENCES file_branch(id) ON DELETE CASCADE DEFERRABLE, @@ -26,6 +37,9 @@ CREATE TABLE IF NOT EXISTS file_branch_change ( changes bytea NOT NULL, + -- the data version the changes were written at + data_version text NOT NULL, + created_at timestamptz NOT NULL DEFAULT clock_timestamp(), updated_at timestamptz NOT NULL DEFAULT clock_timestamp(), deleted_at timestamptz NULL, @@ -33,5 +47,5 @@ CREATE TABLE IF NOT EXISTS file_branch_change ( UNIQUE (file_id, revn) ); -CREATE INDEX IF NOT EXISTS file_branch_change__branch_id__idx +CREATE INDEX file_branch_change__branch_id__idx ON file_branch_change(branch_id) WHERE deleted_at IS NULL; diff --git a/backend/src/app/migrations/sql/0158-add-file-branch-change-data-version.sql b/backend/src/app/migrations/sql/0158-add-file-branch-change-data-version.sql deleted file mode 100644 index 7d9f57c6fb..0000000000 --- a/backend/src/app/migrations/sql/0158-add-file-branch-change-data-version.sql +++ /dev/null @@ -1,18 +0,0 @@ --- The data version each op-log row was written at: the name of the last --- file data migration (`app.common.files.migrations/available-migrations`) --- of the backend that applied the row's changes. A data version is a --- prefix of that list, so its last name identifies it. The branch derive --- (`app.binfile.common/branch-file-data`) replays the log in segments: it --- migrates the document to a row's version before it applies the row, the --- point where an ordinary file migrates across an upgrade. --- --- The merge base needs no column of its own: its snapshot row --- (`file_change.migrations`) records the migrations its data holds. --- --- NULL marks a row written before this column existed. The derive replays --- such a row at the version the document holds, which is the base's --- version because those rows precede every stamped row of their log. That --- is how the derive replayed every row before this column existed. - -ALTER TABLE file_branch_change - ADD COLUMN IF NOT EXISTS data_version text NULL; diff --git a/backend/test/backend_tests/rpc_file_branch_oplog_test.clj b/backend/test/backend_tests/rpc_file_branch_oplog_test.clj index b111ddb732..96a2715a0c 100644 --- a/backend/test/backend_tests/rpc_file_branch_oplog_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_oplog_test.clj @@ -967,44 +967,6 @@ (t/is (= {:name "Two" :path "Brand / Two"} (select-keys (get colors c2) [:name :path])))))))))) -(t/deftest branch-replays-an-unstamped-row-at-the-base-version - ;; A row written before `file_branch_change.data_version` existed has - ;; no stamp. Such rows precede every stamped row and were written over - ;; the base, so the derive replays them at the base's version, as it did - ;; before the column existed. - (with-redefs [cf/flags (conj cf/flags :branching)] - (let [profile (th/create-profile* 1 {:is-active true}) - proj-id (:default-project-id profile) - main (th/create-file* 1 {:profile-id (:id profile) - :project-id proj-id - :is-shared false}) - ordinary (th/create-file* 2 {:profile-id (:id profile) - :project-id proj-id - :is-shared false}) - upgraded (conj fmg/available-migrations test-migration) - c1 (uuid/random) - c2 (uuid/random) - old-op {:type :add-color - :color {:id c1 :name "One" :path "Brand/One" :color "#111111" :opacity 1}} - new-op {:type :add-color - :color {:id c2 :name "Two" :path "Brand / Two" :color "#222222" :opacity 1}} - branch-file-id (:branch-file-id (create-branch* profile (:id main) "unstamped"))] - - (apply-change* profile branch-file-id old-op) - (apply-change* profile (:id ordinary) old-op) - (th/db-update! :file-branch-change {:data-version nil} {:file-id branch-file-id}) - - (with-redefs [fmg/available-migrations upgraded] - (apply-change* profile branch-file-id new-op) - (apply-change* profile (:id ordinary) new-op) - - (t/is (= [nil test-migration] - (->> (oplog-rows branch-file-id) - (sort-by :revn) - (mapv :data-version)))) - (t/is (= (colors-of (read-file profile (:id ordinary))) - (colors-of (read-file profile branch-file-id)))))))) - (t/deftest update-from-main-conflict-sides-name-the-documents ;; `update-branch-from-main` reports its conflicts in document terms: ;; `:main` carries main's value and `:branch` the branch's, whatever the diff --git a/backend/test/backend_tests/rpc_file_branch_test.clj b/backend/test/backend_tests/rpc_file_branch_test.clj index 98adc5d5cc..4786b53ce2 100644 --- a/backend/test/backend_tests/rpc_file_branch_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_test.clj @@ -23,7 +23,6 @@ [app.rpc.commands.files-branch-policies :as fbp] [app.storage :as sto] [app.util.blob :as blob] - [app.util.migrations :as mg] [backend-tests.helpers :as th] [clojure.string :as str] [clojure.test :as t] @@ -2758,67 +2757,6 @@ (compare!) (t/is (= :ignore (get @seen :same-parent-reorder))))))) -(t/deftest re-running-the-branch-migration-leaves-oplog-branches-alone - ;; The branch table migration carries a backfill for the copy model it - ;; predates: rows whose branch file persisted its own data payload. - ;; An op-log branch stores no payload and legitimately starts its own - ;; counter at 0, so a re-run of the file (a renamed re-registration - ;; re-applies every statement) must not touch it, while it still - ;; repositions a copy-model row. - (with-redefs [cf/flags (conj cf/flags :branching)] - (let [profile (th/create-profile* 1 {:is-active true}) - file (th/create-file* 1 {:profile-id (:id profile) - :project-id (:default-project-id profile) - :is-shared false}) - ;; main must have work for the backfill to have anything to - ;; reposition to: a fresh file's revn is 0, and the backfill - ;; copies base_revn, main's revision at branch creation - _ (th/command! {::th/type :update-file - ::rpc/profile-id (:id profile) - :id (:id file) - :session-id (uuid/random) - :revn (:revn (th/db-get :file {:id (:id file)})) - :vern (:vern (th/db-get :file {:id (:id file)})) - :features cfeat/supported-features - :changes [{:type :add-color - :color {:id (uuid/random) - :name "counter" - :color "#ff0000" - :opacity 1}}]}) - oplog-branch (-> (th/command! {::th/type :create-file-branch - ::rpc/profile-id (:id profile) - :file-id (:id file) - :name "oplog-branch"}) - :result) - copy-branch (-> (th/command! {::th/type :create-file-branch - ::rpc/profile-id (:id profile) - :file-id (:id file) - :name "copy-branch"}) - :result) - migrate-again (fn [] - ((mg/resource "app/migrations/sql/0155-add-file-branch-table.sql") - th/*pool*)) - counter (fn [branch-id] - (:base-branch-revn (th/db-get :file-branch {:id branch-id})))] - - ;; both branches start with their own counter at 0 - (t/is (zero? (counter (:id oplog-branch)))) - (t/is (zero? (counter (:id copy-branch)))) - - ;; the copy-model row: its branch file carries a data payload, the - ;; shape the backfill was written for - (th/db-insert! :file-data - {:file-id (:branch-file-id copy-branch) - :id (:branch-file-id copy-branch) - :type "main" - :data (blob/encode {:pages []})}) - (migrate-again) - - ;; the op-log branch keeps its own counter; the copy-model row is - ;; repositioned onto main's revision - (t/is (zero? (counter (:id oplog-branch)))) - (t/is (pos? (counter (:id copy-branch))))))) - (t/deftest summary-cache-key-changes-with-policies ;; Two different policies must never share a cached summary: the key ;; hashes the policies the value was computed under. diff --git a/common/src/app/common/files/migrations.cljc b/common/src/app/common/files/migrations.cljc index 1cddc363d1..fbad7cdd77 100644 --- a/common/src/app/common/files/migrations.cljc +++ b/common/src/app/common/files/migrations.cljc @@ -109,14 +109,12 @@ (d/ordered-set)))) (defn need-migration-to? - "True when `file` has not reached data version `version`. A nil - `version` names no version and needs nothing. A file that already - holds the version's last migration has reached it, which costs one - set lookup." + "True when `file` has not reached data version `version`. A file that + already holds the version's last migration has reached it, which + costs one set lookup." [file version] (let [migrations (:migrations file)] - (and (some? version) - (not (contains? migrations version)) + (and (not (contains? migrations version)) (boolean (->> migrations (set/difference (data-version-migrations version)) diff --git a/common/test/common_tests/files_migrations_test.cljc b/common/test/common_tests/files_migrations_test.cljc index ab7794b5c5..3c89c752d9 100644 --- a/common/test/common_tests/files_migrations_test.cljc +++ b/common/test/common_tests/files_migrations_test.cljc @@ -62,7 +62,6 @@ ctf/check-file-data (fn [_] (throw (ex-info "checked" {})))] (t/is (cfm/need-migration-to? file "test/2")) (t/is (not (cfm/need-migration-to? file "test/1"))) - (t/is (not (cfm/need-migration-to? file nil))) (let [file' (cfm/migrate-file-to file nil "test/2")] (t/is (= 2 (-> file' :data :sum)))