Fix/cascade by parkan · Pull Request #583 · data-preservation-programs/singularity

parkan · 2025-09-17T18:25:58Z

fix postgres fk cascade deadlocks with set null + trigger approach
this addresses the postgres-specific foreign key constraint violations when deleting
preparations by implementing a database-level solution that:

makes files.attachment_id nullable
changes the fk constraint to ON DELETE SET NULL instead of restrict/no action
adds a postgres trigger that immediately deletes files when attachment_id becomes null

this approach keeps the application model unchanged (no pointer types) while solving
the constraint violation. the trigger ensures the app never sees null attachment_id
values so behavior stays consistent.

the solution is postgres-specific because:

postgres has stricter fk enforcement that causes the deadlock/violation issues
mysql tests were already passing with the existing restrict behavior
we removed cascade deletes in 640154e to avoid deadlocks, but that created
the constraint violation when deleting preparations

longer term consideration: files conceptually belong to storages not preparations.
attachments are just prep<->storage associations. we might want to remove this fk
entirely since files should be storage-scoped and reusable across preparations
(expensive to re-scan). this would eliminate the cascade issues entirely while
enabling file reuse when recreating preparations with the same storage.

this resolves two issues that prevent deletion of preparations. fixes #465

having a wallet assigned to a preparation currently blocks its deletion with an unhelpful error message this alters the FK constraints to remove the join table entries if either side is removed

we have a multipath/circular dependency which prevents preparations from being deleted, this removes the FK cascade files -> attachments and should resolve the situation deletions still will happen via the directory transitive FK

parkan · 2025-09-18T19:00:00Z

ok uhhh I have no idea what is going on with the workflows here

parkan · 2025-09-19T01:57:34Z

this is waiting on validation (repro with test cherrypicked into parent branch + clear with my changes 🤞 ), if that works as intended it will be ready for review

parkan · 2025-09-19T01:58:29Z

 					return errors.WithStack(err)
 				}
-				fmt.Printf("Current migration: " + last + "\n")
+                fmt.Printf("Current migration: %s\n", last)


test runner was complaining about this, I'm actually not 100% sure why but it seems happy now

ok this seems to be a legit vet error (non-constant format string potential) 👍

nice catch!

parkan · 2025-09-19T16:15:52Z

ok, test harness working locally in devcontainer, I've identified an issue with the FK pattern, will fix and push

parkan · 2025-09-20T14:17:20Z

running the whole test suite with non-sqlite DBs actually properly enabled results in 294 failures 🤯

this addresses the postgres-specific foreign key constraint violations when deleting preparations by implementing a database-level solution that: 1. makes files.attachment_id nullable 2. changes the fk constraint to ON DELETE SET NULL instead of restrict/no action 3. adds a postgres trigger that immediately deletes files when attachment_id becomes null this approach keeps the application model unchanged (no pointer types) while solving the constraint violation. the trigger ensures the app never sees null attachment_id values so behavior stays consistent. the solution is postgres-specific because: - postgres has stricter fk enforcement that causes the deadlock/violation issues - mysql tests were already passing with the existing restrict behavior - we removed cascade deletes in 640154e to avoid deadlocks, but that created the constraint violation when deleting preparations longer term consideration: files conceptually belong to storages not preparations. attachments are just prep<->storage associations. we might want to remove this fk entirely since files should be storage-scoped and reusable across preparations (expensive to re-scan). this would eliminate the cascade issues entirely while enabling file reuse when recreating preparations with the same storage.

parkan · 2025-09-22T17:25:08Z

ok, I was able to work around the problem, relevant test is passing with both mysql and sqlite backends:

vscode ➜ /workspace (fix/cascade) $ go test ./handler/dataprep -run '^TestRemovePreparationHandler_CascadeCycle_' -v --count=1
=== RUN   TestRemovePreparationHandler_CascadeCycle_Postgres
2025-09-22T17:19:57.884Z        ERROR   database        database/util.go:75     DELETE FROM deal_schedules      {"rowsAffected": 0, "elapsed": 0.000038012, "err": "no such table: deal_schedules"}
    remove_test.go:119: Warning: Failed to clear table deal_schedules: no such table: deal_schedules
=== RUN   TestRemovePreparationHandler_CascadeCycle_Postgres/sqlite
    remove_test.go:121: Skip non-Postgres dialect
2025-09-22T17:19:58.343Z        ERROR   database        database/util.go:75     DELETE FROM deal_schedules      {"rowsAffected": 0, "elapsed": 0.000135886, "err": "Error 1146 (42S02): Table 'test_TestRemovePreparationHandler_CascadeCycle_Postgres_jkokru.deal_schedules' doesn't exist"}
=== NAME  TestRemovePreparationHandler_CascadeCycle_Postgres
    remove_test.go:119: Warning: Failed to clear table deal_schedules: Error 1146 (42S02): Table 'test_TestRemovePreparationHandler_CascadeCycle_Postgres_jkokru.deal_schedules' doesn't exist
=== RUN   TestRemovePreparationHandler_CascadeCycle_Postgres/mysql
    remove_test.go:121: Skip non-Postgres dialect
2025-09-22T17:19:59.036Z        ERROR   database        database/util.go:75     TRUNCATE TABLE deal_schedules CASCADE   {"rowsAffected": 0, "elapsed": 0.000209015, "err": "ERROR: relation \"deal_schedules\" does not exist (SQLSTATE 42P01)"}
=== NAME  TestRemovePreparationHandler_CascadeCycle_Postgres
    remove_test.go:119: Warning: Failed to clear table deal_schedules: ERROR: relation "deal_schedules" does not exist (SQLSTATE 42P01)
=== RUN   TestRemovePreparationHandler_CascadeCycle_Postgres/postgres
--- PASS: TestRemovePreparationHandler_CascadeCycle_Postgres (1.44s)
    --- SKIP: TestRemovePreparationHandler_CascadeCycle_Postgres/sqlite (0.00s)
    --- SKIP: TestRemovePreparationHandler_CascadeCycle_Postgres/mysql (0.00s)
    --- PASS: TestRemovePreparationHandler_CascadeCycle_Postgres/postgres (0.02s)
=== RUN   TestRemovePreparationHandler_CascadeCycle_MySQL
2025-09-22T17:19:59.308Z        ERROR   database        database/util.go:75     DELETE FROM deal_schedules      {"rowsAffected": 0, "elapsed": 0.000018064, "err": "no such table: deal_schedules"}
    remove_test.go:157: Warning: Failed to clear table deal_schedules: no such table: deal_schedules
=== RUN   TestRemovePreparationHandler_CascadeCycle_MySQL/sqlite
    remove_test.go:159: Skip non-MySQL dialect
2025-09-22T17:19:59.715Z        ERROR   database        database/util.go:75     DELETE FROM deal_schedules      {"rowsAffected": 0, "elapsed": 0.000117292, "err": "Error 1146 (42S02): Table 'test_TestRemovePreparationHandler_CascadeCycle_MySQL_itowjh.deal_schedules' doesn't exist"}
=== NAME  TestRemovePreparationHandler_CascadeCycle_MySQL
    remove_test.go:157: Warning: Failed to clear table deal_schedules: Error 1146 (42S02): Table 'test_TestRemovePreparationHandler_CascadeCycle_MySQL_itowjh.deal_schedules' doesn't exist
=== RUN   TestRemovePreparationHandler_CascadeCycle_MySQL/mysql
2025-09-22T17:19:59.977Z        ERROR   database        database/util.go:75     TRUNCATE TABLE deal_schedules CASCADE   {"rowsAffected": 0, "elapsed": 0.000161956, "err": "ERROR: relation \"deal_schedules\" does not exist (SQLSTATE 42P01)"}
=== NAME  TestRemovePreparationHandler_CascadeCycle_MySQL
    remove_test.go:157: Warning: Failed to clear table deal_schedules: ERROR: relation "deal_schedules" does not exist (SQLSTATE 42P01)
=== RUN   TestRemovePreparationHandler_CascadeCycle_MySQL/postgres
    remove_test.go:159: Skip non-MySQL dialect
--- PASS: TestRemovePreparationHandler_CascadeCycle_MySQL (0.83s)
    --- SKIP: TestRemovePreparationHandler_CascadeCycle_MySQL/sqlite (0.00s)
    --- PASS: TestRemovePreparationHandler_CascadeCycle_MySQL/mysql (0.02s)
    --- SKIP: TestRemovePreparationHandler_CascadeCycle_MySQL/postgres (0.00s)
PASS
ok      github.com/data-preservation-programs/singularity/handler/dataprep      2.293s

as you can see our ontology is pretty messy, and as mentioned above actually running the full test suite with DB backends fails miserably, so I'm not going to merge the test harness fixes in this branch, however this specific fix is ready for review

please see details in d2b7aa7 -- there may be a cleaner fix (removing FK entirely from files to attachments) but that is more of a product-level decision

parkan · 2025-09-22T17:27:12Z

+		}
 	case "postgres":
-		connStr = "postgres://singularity:singularity@localhost:5432/singularity?sslmode=disable"
+		connStr = "postgres://postgres@localhost:5432/postgres?sslmode=disable"


this is so that default settings devcontainer postgres feature can be used; it has no overrides like bitnami (running test daemons in devcontainer was the least complex approach I've come up with; it can also be used in github workflows with devcontainers/ci, which I will PR separately)

ianconsolata · 2025-09-30T22:44:41Z

 					return errors.WithStack(err)
 				}
-				fmt.Printf("Current migration: " + last + "\n")
+                fmt.Printf("Current migration: %s\n", last)


nice catch!

resolves several foreign key constraint/cascade issues, primarily unblocking deletion of preparations: - fix join table blocking deletion of preps with attached wallets - fix mysql refusing to delete preprations due to fk cascade parallelogram (#465) - fix postgres fk cascade deadlocks (same issue as above) with set null + trigger approach this addresses the postgres-specific foreign key constraint violations when deleting preparations by implementing a database-level solution that: 1. makes files.attachment_id nullable 2. changes the fk constraint to ON DELETE SET NULL instead of restrict/no action 3. adds a postgres trigger that immediately deletes files when attachment_id becomes null this approach keeps the application model unchanged (no pointer types) while solving the constraint violation. the trigger ensures the app never sees null attachment_id values so behavior stays consistent. the solution is postgres-specific because: - postgres has stricter fk enforcement that causes the deadlock/violation issues - mysql tests were already passing with the existing restrict behavior - we removed cascade deletes in 640154e to avoid deadlocks, but that created the constraint violation when deleting preparations longer term consideration: files conceptually belong to storages not preparations. attachments are just prep<->storage associations. we might want to remove this fk entirely since files should be storage-scoped and reusable across preparations (expensive to re-scan). this would eliminate the cascade issues entirely while enabling file reuse when recreating preparations with the same storage. this resolves two issues that prevent deletion of preparations. fixes #465

parkan added 2 commits September 17, 2025 14:05

ON DELETE CASCADE for wallet_assignments

057d986

having a wallet assigned to a preparation currently blocks its deletion with an unhelpful error message this alters the FK constraints to remove the join table entries if either side is removed

parkan mentioned this pull request Sep 18, 2025

[Bug]: prep remove does not work with mysql database #465

Closed

parkan requested a review from ianconsolata September 18, 2025 18:28

parkan force-pushed the fix/cascade branch 2 times, most recently from 0901be4 to b86b02d Compare September 19, 2025 01:39

test for prep removal

b92d52f

parkan force-pushed the fix/cascade branch from b86b02d to b92d52f Compare September 19, 2025 01:45

parkan commented Sep 19, 2025

View reviewed changes

use default postgres user in tests for ease

5ee6093

parkan marked this pull request as ready for review September 22, 2025 17:25

parkan commented Sep 22, 2025

View reviewed changes

parkan mentioned this pull request Sep 22, 2025

Experimental test workflow with devcontainer #585

Closed

ianconsolata approved these changes Sep 30, 2025

View reviewed changes

explanatory comments

0b0aefe

parkan merged commit d64eaab into develop Oct 2, 2025
13 checks passed

parkan deleted the fix/cascade branch October 2, 2025 10:34

parkan mentioned this pull request Oct 28, 2025

chore/update #589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/cascade#583

Fix/cascade#583
parkan merged 6 commits into
developfrom
fix/cascade

parkan commented Sep 17, 2025 •

edited

Loading

Uh oh!

parkan commented Sep 18, 2025

Uh oh!

parkan commented Sep 19, 2025

Uh oh!

parkan Sep 19, 2025

Uh oh!

parkan Sep 19, 2025 •

edited

Loading

Uh oh!

ianconsolata Sep 30, 2025

Uh oh!

parkan commented Sep 19, 2025

Uh oh!

parkan commented Sep 20, 2025 •

edited

Loading

Uh oh!

parkan commented Sep 22, 2025 •

edited

Loading

Uh oh!

parkan Sep 22, 2025 •

edited

Loading

Uh oh!

ianconsolata Sep 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

parkan commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parkan commented Sep 18, 2025

Uh oh!

parkan commented Sep 19, 2025

Uh oh!

parkan Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

parkan Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianconsolata Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

parkan commented Sep 19, 2025

Uh oh!

parkan commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parkan commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parkan Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianconsolata Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

parkan commented Sep 17, 2025 •

edited

Loading

parkan Sep 19, 2025 •

edited

Loading

parkan commented Sep 20, 2025 •

edited

Loading

parkan commented Sep 22, 2025 •

edited

Loading

parkan Sep 22, 2025 •

edited

Loading