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

refactor(core): Don't use DB transactions on ExecutionRepository.createNewExecution #8002

Merged
merged 3 commits into from
Dec 12, 2023

Conversation

netroy
Copy link
Member

@netroy netroy commented Dec 12, 2023

Summary

Saving execution data is one of the slowest DB operations in the application, and is likely behind some of the sqlite transaction concurrency issues we've been seeing.
This not only remove the 2 separate transactions for saving ExecutionEntity and ExecutionData, but also remove fields from ExecutionData.workflowData that don't need to be saved (like tags, shared, statistics, triggerCount, etc).

Review / Merge checklist

  • PR title and summary are descriptive.
  • Tests included.
  • DB Tests

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Dec 12, 2023
@netroy netroy force-pushed the no-transaction-on-exectution-save branch from c9bf57e to 06c4c6e Compare December 12, 2023 10:47
@netroy netroy requested a review from krynble December 12, 2023 11:26
@netroy
Copy link
Member Author

netroy commented Dec 12, 2023

latest DB Tests

krynble
krynble previously approved these changes Dec 12, 2023
Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

cypress bot commented Dec 12, 2023

1 flaky test on run #3310 ↗︎

0 294 5 0 Flakiness 1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*
Project: n8n Commit: 6296685b99
Status: Passed Duration: 06:31 💡
Started: Dec 12, 2023 1:03 PM Ended: Dec 12, 2023 1:09 PM
Flakiness  cypress/e2e/32-node-io-filter.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Node IO Filter > should filter input/output data separately Screenshots Video

Review all test suite changes for PR #8002 ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

Copy link
Contributor

✅ All Cypress E2E specs passed

@netroy netroy merged commit 1d87041 into master Dec 12, 2023
19 checks passed
@netroy netroy deleted the no-transaction-on-exectution-save branch December 12, 2023 13:36
@janober
Copy link
Member

janober commented Dec 13, 2023

Got released with n8n@1.21.0

netroy added a commit that referenced this pull request Dec 15, 2023
…teNewExecution (#8002)

Saving execution data is one of the slowest DB operations in the
application, and is likely behind some of the sqlite transaction
concurrency issues we've been seeing.
This not only remove the 2 separate transactions for saving
`ExecutionEntity` and `ExecutionData`, but also remove fields from
`ExecutionData.workflowData` that don't need to be saved (like `tags`,
`shared`, `statistics`, `triggerCount`, etc).
@netroy netroy mentioned this pull request Dec 15, 2023
netroy added a commit that referenced this pull request Dec 15, 2023
## [1.18.3](https://github.com/n8n-io/n8n/compare/n8n@1.18.2...n8n@1.18.3) (2023-12-15)

### Refactor

* **Core:** Don't use DB transactions on `ExecutionRepository.createNewExecution` ([#8002](#8002)) ([f50c14b](f50c14b))

Co-authored-by: netroy <netroy@users.noreply.github.com>
ivov added a commit that referenced this pull request Dec 15, 2023
##
[1.21.1](https://github.com/n8n-io/n8n/compare/n8n@1.21.0...n8n@1.21.1)
(2023-12-15)


### Bug Fixes

* **ActiveCampaign Node:** Fix pagination issue when loading tags
([#8017](#8017))
([d661861](d661861))
* **core:** Initialize queue once in queue mode
([#8025](#8025))
([b1c9c50](b1c9c50))
* **core:** Restore workflow ID during execution creation
([#8031](#8031))
([2d16161](2d16161)),
closes
[/github.com//pull/8002/files#diff-c8cbb62ca9ab2ae45e5f565cd8c63fff6475809a6241ea0b90acc575615224](https://github.com//github.com/n8n-io/n8n/pull/8002/files/issues/diff-c8cbb62ca9ab2ae45e5f565cd8c63fff6475809a6241ea0b90acc575615224)
* **editor:** Add back credential `use` permission
([#8023](#8023))
([cf5c723](cf5c723))
* **editor:** Disable auto scroll and list size check when clicking on
executions ([#7983](#7983))
([99d1771](99d1771))
* **editor:** Show credential share info only to appropriate users
([#8020](#8020))
([9933fce](9933fce))
* **editor:** Turn off executions list auto-refresh after leaving the
page ([#8005](#8005))
([b866a94](b866a94))

Co-authored-by: ivov <ivov@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants