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

fix(mssql): force transactions if attempting to insert more than 1,000 rows #15458

Merged
merged 3 commits into from Dec 18, 2022

Conversation

evanrittenhouse
Copy link
Member

@evanrittenhouse evanrittenhouse commented Dec 15, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description Of Change

Force a transaction to be used if we're attempting to insert more than 1,000 rows in a single bulkInsert. This allows us to safely rollback everything in case it fails.

This fixes #15426.

@evanrittenhouse evanrittenhouse requested review from ephys and a team and removed request for a team and ephys December 15, 2022 04:22
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

A few notes but overall looks good :)

src/dialects/mssql/query-interface.js Outdated Show resolved Hide resolved
test/integration/dialects/mssql/regressions.test.js Outdated Show resolved Hide resolved
test/integration/dialects/mssql/regressions.test.js Outdated Show resolved Hide resolved
src/dialects/mssql/query-interface.js Outdated Show resolved Hide resolved
@evanrittenhouse evanrittenhouse force-pushed the mssql_bulkinsert branch 2 times, most recently from e2f4e11 to ecd9fcc Compare December 17, 2022 06:02
ephys
ephys previously approved these changes Dec 17, 2022
@ephys
Copy link
Member

ephys commented Dec 17, 2022

Ah, looks like this change causes an unexpected deadlock?

@evanrittenhouse
Copy link
Member Author

Yep, need to investigate why - it passes the integration tests locally which is a bit odd, so I'll have to check into what's going on with our GH action.

@evanrittenhouse
Copy link
Member Author

Converting to draft since I can't get the test failures repro'd locally - will need to push a lot

@WikiRik WikiRik changed the title fix: Force transactions in MSSQL if attempting to insert more than 1,000 rows fix(mssql): force transactions if attempting to insert more than 1,000 rows Dec 17, 2022
WikiRik
WikiRik previously approved these changes Dec 17, 2022
@WikiRik
Copy link
Member

WikiRik commented Dec 17, 2022

I was interested by the failing tests and it's a weird one.
Where I ended up is that the difference between the old code and the current code is in this line;

https://github.com/evanrittenhouse/sequelize/blob/9ffe2b1a2d95a61b559aa4d58ce542c2f6577c10/src/model.js#L2780

The inputs (associationInstances and includeOptions) are the same, but instances before running that is the same in both versions but instances after that is different.

I'm using the following test to debug;
https://github.com/evanrittenhouse/sequelize/blob/9ffe2b1a2d95a61b559aa4d58ce542c2f6577c10/test/integration/model/bulk-create/include.test.js#L12-L84

In this case the UserId in Product is null instead of 1 or 2, which causes the test to fail. Weird stuff

@evanrittenhouse
Copy link
Member Author

@WikiRik you're saying the same inputs are applied to the same input array but are yielding different results?

@evanrittenhouse evanrittenhouse marked this pull request as ready for review December 18, 2022 19:28
@evanrittenhouse evanrittenhouse merged commit cda75b9 into sequelize:main Dec 18, 2022
@evanrittenhouse evanrittenhouse deleted the mssql_bulkinsert branch December 18, 2022 20:49
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.

bulkCreate does not fully rollback in mssql when inserting more than 1000 items
3 participants