Skip to content
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

:on-delete :cascade #698

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

:on-delete :cascade #698

wants to merge 4 commits into from

Conversation

tonsky
Copy link
Contributor

@tonsky tonsky commented Jan 9, 2025

  • Optimize current on-delete impl
  • Transform it into single recursive query
  • Add delete-entity support to client datalog
  • Add option to the explorer
  • Add option to CLI schema file

@tonsky tonsky force-pushed the optimize-cascade-delete branch from 3a084cd to 193d5af Compare January 9, 2025 20:06
Copy link

github-actions bot commented Jan 9, 2025

View Vercel preview at instant-www-js-optimize-cascade-delete-jsv.vercel.app.

@tonsky
Copy link
Contributor Author

tonsky commented Jan 9, 2025

Progress so far:

  • Wrote a bench for recursive delete
  • Optimized delete-entity-multi!

@tonsky tonsky force-pushed the optimize-cascade-delete branch 2 times, most recently from b9f9191 to 0d35b0c Compare January 9, 2025 20:12
@tonsky tonsky force-pushed the optimize-cascade-delete branch from 0d35b0c to 293ab5b Compare January 16, 2025 21:12
server/src/instant/db/transaction.clj Outdated Show resolved Hide resolved
(assoc acc :deep-merge-triple (triple-model/deep-merge-multi! conn attrs app-id args))

:retract-triple
(assoc acc :retract-triple (triple-model/delete-multi! conn app-id args))))
{}
(batch tx-steps))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this might be simpler if batch did a group-by instead of whatever it's currently doing.

server/src/instant/db/transaction.clj Show resolved Hide resolved
server/src/instant/db/transaction.clj Outdated Show resolved Hide resolved
:update-attr
(assoc acc :update-attr (attr-model/update-multi! conn app-id args))

:delete-entity-no-cascade
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have :delete-entity-no-cascade as an operation? Do we expect users to use it or is it just for our internal usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is that we don’t try to expand :delete-entity second time, since some methods call transact-without-tx-conn! directly and some permissioned-transaction/transact!

@tonsky tonsky force-pushed the optimize-cascade-delete branch 2 times, most recently from db7d1ee to e2b34a9 Compare January 17, 2025 20:02
@tonsky
Copy link
Contributor Author

tonsky commented Jan 17, 2025

To the best of my abilities, I made cascade expansion as a single query but it still sometimes triggers slow path.

One of the culprits, I think, is [:= [:to_jsonb :entity_id] :value] join. This could be sped up by introducing value_ref column that contains parsed uuid. Might benefit many other places, too, anywhere where we need to follow a reference, in fact.

I left a TODO, maybe plan it as a separate task?

@tonsky tonsky force-pushed the optimize-cascade-delete branch from e2b34a9 to 3a7869c Compare January 17, 2025 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants