-
Notifications
You must be signed in to change notification settings - Fork 91
POC Row event sequence stamping for submissions #1699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
27 changes: 27 additions & 0 deletions
27
lib/model/migrations/20251127-01-submission-event-stamping-01.down.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| -- Copyright 2025 ODK Central Developers | ||
| -- See the NOTICE file at the top-level directory of this distribution and at | ||
| -- https://github.com/getodk/central-backend/blob/master/NOTICE. | ||
| -- This file is part of ODK Central. It is subject to the license terms in | ||
| -- the LICENSE file found in the top-level directory of this distribution and at | ||
| -- https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, | ||
| -- including this file, may be copied, modified, propagated, or distributed | ||
| -- except according to the terms contained in the LICENSE file. | ||
|
|
||
|
|
||
| --- drop: get_event(submission_id integer) --- | ||
| DROP FUNCTION IF EXISTS "public"."get_event"(submission_id integer) CASCADE; | ||
|
|
||
| --- drop: public.submissions.set_eventstamp_submissions_at_commit --- | ||
| DROP TRIGGER IF EXISTS set_eventstamp_submissions_at_commit ON "public"."submissions" CASCADE; | ||
|
|
||
| --- drop: public.submissions.blank_submissions_event_on_insert --- | ||
| DROP TRIGGER IF EXISTS blank_submissions_event_on_insert ON "public"."submissions" CASCADE; | ||
|
|
||
| --- drop: eventstamp_submissions_triggerfunction --- | ||
| DROP FUNCTION IF EXISTS "public"."eventstamp_submissions_triggerfunction"() CASCADE; | ||
|
|
||
| --- drop: blank_submissions_event_triggerfunction --- | ||
| DROP FUNCTION IF EXISTS "public"."blank_submissions_event_triggerfunction"() CASCADE; | ||
|
|
||
| --- drop: public.submissions.blank_submissions_event_on_update --- | ||
| DROP TRIGGER IF EXISTS blank_submissions_event_on_update ON "public"."submissions" CASCADE; |
36 changes: 36 additions & 0 deletions
36
lib/model/migrations/20251127-01-submission-event-stamping-01.up.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| -- Copyright 2025 ODK Central Developers | ||
| -- See the NOTICE file at the top-level directory of this distribution and at | ||
| -- https://github.com/getodk/central-backend/blob/master/NOTICE. | ||
| -- This file is part of ODK Central. It is subject to the license terms in | ||
| -- the LICENSE file found in the top-level directory of this distribution and at | ||
| -- https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, | ||
| -- including this file, may be copied, modified, propagated, or distributed | ||
| -- except according to the terms contained in the LICENSE file. | ||
|
|
||
| ALTER TABLE submissions | ||
| ADD COLUMN event bigint | ||
| ; | ||
|
|
||
| CREATE INDEX event_idx ON submissions (event) | ||
| ; | ||
|
|
||
| UPDATE | ||
| submissions s1 | ||
| SET | ||
| event = s2.rowno | ||
| FROM ( | ||
| SELECT | ||
| s_inner.id, | ||
| row_number() OVER (ORDER BY COALESCE(s_inner."updatedAt", s_inner."createdAt"), s_inner.id) AS rowno | ||
| FROM | ||
| submissions s_inner) AS s2 | ||
| WHERE | ||
| s1.id = s2.id | ||
| ; | ||
|
|
||
| CREATE TABLE current_event ( | ||
| event bigint NOT NULL | ||
| ) | ||
| ; | ||
|
|
||
| INSERT INTO current_event (event) (SELECT coalesce(MAX(event), 0) FROM submissions); | ||
11 changes: 11 additions & 0 deletions
11
lib/model/migrations/20251127-01-submission-event-stamping-02.down.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| -- Copyright 2025 ODK Central Developers | ||
| -- See the NOTICE file at the top-level directory of this distribution and at | ||
| -- https://github.com/getodk/central-backend/blob/master/NOTICE. | ||
| -- This file is part of ODK Central. It is subject to the license terms in | ||
| -- the LICENSE file found in the top-level directory of this distribution and at | ||
| -- https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, | ||
| -- including this file, may be copied, modified, propagated, or distributed | ||
| -- except according to the terms contained in the LICENSE file. | ||
|
|
||
| DROP TABLE current_event; | ||
| ALTER TABLE submissions DROP COLUMN event; |
126 changes: 126 additions & 0 deletions
126
lib/model/migrations/20251127-01-submission-event-stamping-02.up.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| -- Copyright 2025 ODK Central Developers | ||
| -- See the NOTICE file at the top-level directory of this distribution and at | ||
| -- https://github.com/getodk/central-backend/blob/master/NOTICE. | ||
| -- This file is part of ODK Central. It is subject to the license terms in | ||
| -- the LICENSE file found in the top-level directory of this distribution and at | ||
| -- https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, | ||
| -- including this file, may be copied, modified, propagated, or distributed | ||
| -- except according to the terms contained in the LICENSE file. | ||
|
|
||
|
|
||
| --- create: blank_submissions_event_triggerfunction --- | ||
| CREATE FUNCTION "public"."blank_submissions_event_triggerfunction"() | ||
| RETURNS trigger | ||
| AS | ||
| $BODY$ | ||
| BEGIN | ||
| NEW.event = NULL; | ||
| RETURN NEW; | ||
| END; | ||
| $BODY$ | ||
| LANGUAGE plpgsql | ||
| ; | ||
|
|
||
| --- sign: blank_submissions_event_triggerfunction --- | ||
| COMMENT ON FUNCTION "public"."blank_submissions_event_triggerfunction"() IS '{"dbsamizdat": {"version": 1, "definition_hash": "0982b1119302eb5c8afe455903c5ec8a"}}'; | ||
|
|
||
| --- create: get_event --- | ||
| CREATE FUNCTION "public"."get_event"() | ||
| RETURNS bigint | ||
| AS | ||
| $BODY$ | ||
| -- Locks out invocations in other transactions until this transaction commits. | ||
| -- Use at the end of a transaction (the idea is to use this in a deferred constraint trigger). | ||
|
|
||
| WITH current_event_locked AS ( | ||
| SELECT event FROM current_event | ||
| FOR UPDATE | ||
| ), | ||
|
|
||
| -- figure out whether we've already acquired the advisory lock. | ||
| -- The lock indicates whether, within this transaction, we've already bumped the eventcounter, and thus shouldn't do it again. | ||
| -- (because this function is called in a constraint trigger which automatically runs at commit time, for every new/modified row) | ||
| test_lock AS ( | ||
|
brontolosone marked this conversation as resolved.
|
||
| SELECT | ||
| EXISTS ( | ||
| SELECT | ||
| 1 | ||
| FROM | ||
| -- the `pg_locks` catalog is cluster-wide, so we need to select for the current database | ||
| pg_locks l INNER JOIN pg_database d ON (d.datname = current_database() AND d.oid = l.database) | ||
| WHERE | ||
| -- See PostgreSQL documentation (for version 14), section 52.74: | ||
| -- https://www.postgresql.org/docs/14/view-pg-locks.html#:~:text=A%20bigint%20key%20is%20displayed%20with%20its%20high%2Dorder%20half%20in%20the%20classid%20column%2C%20its%20low%2Dorder%20half%20in%20the%20objid%20column%2C%20and%20objsubid%20equal%20to%201%2E | ||
| l.locktype = 'advisory' | ||
| AND l.objsubid = 1 | ||
| AND ((l.classid::bigint << 32) | l.objid::bigint) = hash_text_to_bigint(pg_current_xact_id()::text, 'submissions-eventstamp-lock') | ||
| ) as lockfound, | ||
| pg_advisory_xact_lock(hash_text_to_bigint(pg_current_xact_id()::text, 'submissions-eventstamp-lock')) AS newlock | ||
| ), | ||
|
|
||
| maybe_new_event AS ( | ||
| UPDATE current_event | ||
| SET event = event + 1 | ||
| FROM test_lock | ||
| WHERE test_lock.lockfound = false -- the lock would mean that we're not processing the first row (of potentially many) in this transaction, and the eventcounter is already incremented | ||
| RETURNING event | ||
| ) | ||
|
|
||
| SELECT COALESCE( | ||
| (SELECT event FROM maybe_new_event), | ||
| (SELECT event FROM current_event_locked) | ||
| ) as event | ||
|
|
||
| $BODY$ | ||
| LANGUAGE sql | ||
| ; | ||
|
|
||
| --- sign: get_event --- | ||
| COMMENT ON FUNCTION "public"."get_event"() IS '{"dbsamizdat": {"version": 1, "definition_hash": "dd6ab7bb6eef6087a892360fd85f4151"}}'; | ||
|
|
||
| --- create: eventstamp_submissions_triggerfunction --- | ||
| CREATE FUNCTION "public"."eventstamp_submissions_triggerfunction"() | ||
| RETURNS trigger | ||
| AS | ||
| $BODY$ | ||
| BEGIN | ||
| -- as this is called from a constraint trigger, we can't modify the data through modification of NEW. | ||
| UPDATE submissions SET event = get_event() WHERE id = NEW.id; | ||
| RETURN NEW; | ||
| END; | ||
| $BODY$ | ||
| LANGUAGE plpgsql | ||
| ; | ||
|
|
||
| --- sign: eventstamp_submissions_triggerfunction --- | ||
| COMMENT ON FUNCTION "public"."eventstamp_submissions_triggerfunction"() IS '{"dbsamizdat": {"version": 1, "definition_hash": "9af0a3a329872908cdf8ad0e7fc34efd"}}'; | ||
|
|
||
| --- create: public.submissions.blank_submissions_event_on_update --- | ||
| CREATE TRIGGER "blank_submissions_event_on_update" BEFORE UPDATE ON "public"."submissions" | ||
| -- Application transparency: | ||
| -- New rows are already created with a NULL event stamp by default. | ||
| -- Updates to rows need to have their event stamp reset to NULL as well. | ||
| FOR EACH ROW | ||
| WHEN ( | ||
| (NEW.event IS NOT NULL) | ||
| AND | ||
| (OLD.event IS NOT NULL) | ||
| ) | ||
| EXECUTE PROCEDURE blank_submissions_event_triggerfunction() | ||
| ; | ||
|
|
||
| --- sign: public.submissions.blank_submissions_event_on_update --- | ||
| COMMENT ON TRIGGER "blank_submissions_event_on_update" ON "public"."submissions" IS '{"dbsamizdat": {"version": 1, "definition_hash": "c6d9c9e5191fa009a63a4155620f8eb2"}}'; | ||
|
|
||
| --- create: public.submissions.set_eventstamp_submissions_at_commit --- | ||
| CREATE CONSTRAINT TRIGGER "set_eventstamp_submissions_at_commit" AFTER INSERT OR UPDATE ON "public"."submissions" | ||
| -- Runs at the end of a transaction. | ||
| -- The `NEW.event IS NULL` filter prevents recursion. | ||
| DEFERRABLE INITIALLY DEFERRED | ||
| FOR EACH ROW | ||
| WHEN (NEW.event IS NULL) | ||
| EXECUTE PROCEDURE eventstamp_submissions_triggerfunction(); | ||
| ; | ||
|
|
||
| --- sign: public.submissions.set_eventstamp_submissions_at_commit --- | ||
| COMMENT ON TRIGGER "set_eventstamp_submissions_at_commit" ON "public"."submissions" IS '{"dbsamizdat": {"version": 1, "definition_hash": "9bc8ef789e6e8cd458dccc98b88b0db9"}}'; | ||
10 changes: 10 additions & 0 deletions
10
lib/model/migrations/20251127-01-submission-event-stamping.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| // Copyright 2025 ODK Central Developers | ||
| // See the NOTICE file at the top-level directory of this distribution and at | ||
| // https://github.com/getodk/central-backend/blob/master/NOTICE. | ||
| // This file is part of ODK Central. It is subject to the license terms in | ||
| // the LICENSE file found in the top-level directory of this distribution and at | ||
| // https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, | ||
| // including this file, may be copied, modified, propagated, or distributed | ||
| // except according to the terms contained in the LICENSE file. | ||
|
|
||
| module.exports = require('../pure-sql-migration')(__filename); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| const { testService } = require('../setup'); | ||
| const { sql } = require('slonik'); | ||
| const { instances } = require('../../data/xml'); | ||
|
|
||
|
|
||
| describe('DB: Event stamping', () => { | ||
|
|
||
| it('Event stamps are applied upon submission insertion and modification', testService(async (service, { db }) => { | ||
| // As we are testing for the effect of a commit, we have to... commit. And then clean up. | ||
| try { | ||
| const asAlice = await service.login('alice'); | ||
|
|
||
| await asAlice.post('/v1/projects/1/forms/simple/submissions') | ||
| .set('Content-Type', 'application/xml') | ||
| .send(instances.simple.one) | ||
| .expect(200); | ||
|
|
||
| await asAlice.post('/v1/projects/1/forms/simple/submissions') | ||
| .set('Content-Type', 'application/xml') | ||
| .send(instances.simple.two) | ||
| .expect(200); | ||
|
|
||
| await db.query(sql`commit;`); | ||
| // .one and .two were created in the same transaction, so they should receive the same event stamp | ||
| (await db.oneFirst(sql`select distinct(event) from submissions`)).should.equal(1); | ||
|
|
||
| await asAlice.post('/v1/projects/1/forms/simple/submissions') | ||
| .set('Content-Type', 'application/xml') | ||
| .send(instances.simple.three) | ||
| .expect(200); | ||
|
|
||
| await db.query(sql`commit;`); | ||
| (await db.oneFirst(sql`select event from submissions where "instanceId" = 'three'`)).should.equal(2); | ||
|
|
||
| await asAlice.put('/v1/projects/1/forms/simple/submissions/one') | ||
| .set('Content-Type', 'application/xml') | ||
| .send('<data id="simple"><meta><instanceID>oneA</instanceID><deprecatedID>one</deprecatedID></meta><whatevs>bla</whatevs></data>') | ||
| .expect(200); | ||
|
|
||
| await db.query(sql`commit;`); | ||
| (await db.oneFirst(sql`select event from submissions where "instanceId" = 'one'`)).should.equal(3); | ||
|
|
||
| await asAlice.patch('/v1/projects/1/forms/simple/submissions/one') | ||
| .set('Content-Type', 'application/json') | ||
| .send({ reviewState: 'approved' }) | ||
| .expect(200); | ||
|
|
||
| await db.query(sql`commit;`); | ||
| (await db.oneFirst(sql`select event from submissions where "instanceId" = 'one'`)).should.equal(4); | ||
|
|
||
| } finally { | ||
| await db.query(sql`truncate table submissions cascade`); | ||
| await db.query(sql`truncate table audits cascade`); | ||
| await db.query(sql`commit;`); | ||
| } | ||
|
|
||
| })); | ||
|
|
||
| }); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we will add this mechanism for other tables then what will be the value of this? I am assuming it will be just existing value + number of rows in that table
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to us. We can choose to pretend that all rows in that other table were added at the same time (which is not what I did in the migration), in which case it'd be existing value + 1, or we can indeed do what you suggest.
Another thing we could do is to use 0 for everything that needs to be retroactively eventstamped. But then we'd impact the ability to use it as a cursor - the first chunk of a collection, for retro-applied eventstamps (existing deployments), for event 0, will be potentially huge. On the plus side we'd gain some measure of causal consistency, eg a submission won't exist "before" the project it belongs to. But that causal ordering can break anyway since only few of our tables are append-only (a project can be modified, in which case its eventstamp is going to be higher than that of any of the submissions that have been made for it, until those get modified).