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

Transactions for SQL Server #6972

Merged
merged 7 commits into from Dec 21, 2016

Conversation

5 participants
@harshithkashyap
Member

harshithkashyap commented Dec 10, 2016

Pull Request check-list

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does your issue contain a link to existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Have you added an entry under Future in the changelog?

Description of change

  • Based on @mbroadst suggestion, generateTransactionId method is added which uses crypto.randomBytes and uuid.v4 in case of other dialects. Closes #3950
  • Enabled tests for transactions for mssql dialect
  • Added SAVE TRANSACTION check to call tedious.saveTransaction method
  • Fix tests which are skipped
@mention-bot

This comment has been minimized.

mention-bot commented Dec 10, 2016

@harshithkashyap, thanks for your PR! By analyzing the history of the files in this pull request, we identified @BridgeAR, @felixfbecker and @mbroadst to be potential reviewers.

@harshithkashyap

This comment has been minimized.

Member

harshithkashyap commented Dec 10, 2016

@felixfbecker @janmeier I've skipped a lot of tests which were timing out. By default, SQL Server blocks even select calls if the row has an exclusive lock. Most of the tests in the codebase fails as transaction is never committed/rolledback.

How do you guys prefer to fix this? I'm checking if the dialect is mssql and using Promise.resolve to skip the select statement in some tests. There are a lot of tests based on this approach as Postgresql and Mysql only blocks writes and allows reads.

@codecov-io

This comment has been minimized.

codecov-io commented Dec 10, 2016

Current coverage is 94.80% (diff: 95.45%)

No coverage report found for master at cc09a3c.

Powered by Codecov. Last update cc09a3c...28fce11

@harshithkashyap harshithkashyap force-pushed the harshithkashyap:mssql-tran-name-fix branch from 95c4706 to 59685a0 Dec 10, 2016

@harshithkashyap harshithkashyap changed the title from Trasactions for SQL Server to Transactions for SQL Server Dec 10, 2016

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Dec 10, 2016

Well cheating the tests is not an option of course. Maybe just commit the transaction before the SELECT?

@harshithkashyap

This comment has been minimized.

Member

harshithkashyap commented Dec 10, 2016

But would that not cause the subsequent then calls to fail as it tries to use a transaction which is already committed?

@harshithkashyap

This comment has been minimized.

Member

harshithkashyap commented Dec 11, 2016

@harshithkashyap harshithkashyap force-pushed the harshithkashyap:mssql-tran-name-fix branch 2 times, most recently from 8317577 to aea2138 Dec 17, 2016

Added READ_COMMITTED_SNAPSHOT ON script for tests, fixes skipped test…
…s, transaction savepoints, uses sequelize_test as default database on appveyor

@harshithkashyap harshithkashyap force-pushed the harshithkashyap:mssql-tran-name-fix branch from aea2138 to b759355 Dec 17, 2016

@harshithkashyap

This comment has been minimized.

Member

harshithkashyap commented Dec 17, 2016

@felixfbecker @janmeier SQL Server allows READ_COMMITTED_SNAPSHOT option to allow reads during transactions. I've enabled back all the tests and all of the pass in my localhost.
READ_COMMITTED_SNAPSHOT must be turned on before we can use it and it cannot be enabled on master database. I've changed the database in appveyor-setup.ps1 to sequelize_test but i think it has to be manually created on your end. Could you guys please enable it and review this PR?

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Dec 17, 2016

@harshithkashyap What do you mean with "it has to be enabled on our end"? AppVeyor is 100% configured through appveyor.yml and appveyor-setup.ps1

@harshithkashyap

This comment has been minimized.

Member

harshithkashyap commented Dec 17, 2016

I've changed the database from master to sequelize_test here. I expected appveyor to create the sequelize_test database automatically with the credentials from appveyor-setup.ps1 file.
Login failed error is thrown in the build.

@harshithkashyap harshithkashyap force-pushed the harshithkashyap:mssql-tran-name-fix branch from 762b742 to 8c10a10 Dec 17, 2016

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Dec 17, 2016

hm no idea, im no mssql expert

@harshithkashyap

This comment has been minimized.

Member

harshithkashyap commented Dec 17, 2016

@felixfbecker I've fixed it using appveyor-setup.ps1 script

@harshithkashyap harshithkashyap referenced this pull request Dec 17, 2016

Merged

Transactions for SQL Server - V3 backport #7001

5 of 5 tasks complete
@harshithkashyap

This comment has been minimized.

Member

harshithkashyap commented Dec 20, 2016

@janmeier

LGTM, good work

Out of curiousity, did you figure out why we have to call the beginTransaction et.al. methods, instead of just sending the sql command?

}, this.options.transaction.name);
} else if (_.includes(this.sql, 'SAVE TRANSACTION')) {
connection.saveTransaction(err => {
if (!!err) {

This comment has been minimized.

@felixfbecker

felixfbecker Dec 20, 2016

Contributor

unneeded boolean cast

This comment has been minimized.

@harshithkashyap

harshithkashyap Dec 20, 2016

Member

@felixfbecker beginTransaction, rollbackTransaction and commitTransaction also has boolean cast. Should i remove them aswell?

This comment has been minimized.

@felixfbecker

felixfbecker Dec 20, 2016

Contributor

Feel free to do so. The desired coding style is defined in eslintrc.json, it's just that the codebase does not fully comply with it yet.

@@ -847,7 +847,7 @@ class QueryInterface {
options = _.assign({}, options, {
transaction: transaction.parent || transaction
});
options.transaction.name = transaction.parent ? transaction.name: undefined;

This comment has been minimized.

@felixfbecker

felixfbecker Dec 20, 2016

Contributor

space before colon

@@ -898,7 +898,7 @@ class QueryInterface {
transaction: transaction.parent || transaction,
supportsSearchPath: false
});
options.transaction.name = transaction.parent ? transaction.name: undefined;

This comment has been minimized.

@felixfbecker

felixfbecker Dec 20, 2016

Contributor

space before colon

@harshithkashyap

This comment has been minimized.

Member

harshithkashyap commented Dec 20, 2016

@janmeier Without tedious methods, sql server throws

Transaction count after EXECUTE indicates a mismatching number of BEGIN and COMMIT statements. Previous count = 0, current count = 1.

We use connection.execSql(request); here. When beginTransaction and other methods are called, tedious internally calls connection.execSqlBatch instead of connection.execSql.

Using connection.execSqlBatch allows us to execute sequelize generated queries. All tests seems to pass aswell.

From the tedious docs, it seems that SQL Server will not reuse the execution plan if the same queries are executed again and again. This might add overhead to non transaction queries.

@@ -29,7 +29,6 @@ module.exports = {
host: process.env.SEQ_MSSQL_HOST || process.env.SEQ_HOST || 'mssql.sequelizejs.com',
port: process.env.SEQ_MSSQL_PORT || process.env.SEQ_PORT || 1433,
dialectOptions: {
instanceName: process.env.MSSQL_INSTANCE ? process.env.MSSQL_INSTANCE : 'SQLEXPRESS',

This comment has been minimized.

@felixfbecker

felixfbecker Dec 20, 2016

Contributor

Why is this not needed anymore?

This comment has been minimized.

@harshithkashyap

harshithkashyap Dec 20, 2016

Member

By default instance names are unnamed. I cannot connect to my local server even after specifying the server name and port. And it doesn't default to SQLEXPRESS

This comment has been minimized.

@felixfbecker

felixfbecker Dec 20, 2016

Contributor

Okay, was just curious. I'm not familiar with MSSQL, I just ported the thing over to AppVeyor and copied a lot from tedious PowerShell setup script.

@janmeier janmeier merged commit 086255e into sequelize:master Dec 21, 2016

4 checks passed

codecov/patch 95.45% of diff hit (target 94.80%)
Details
codecov/project 94.80% (+0.10%) compared to 4214143
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@janmeier

This comment has been minimized.

Member

janmeier commented Dec 21, 2016

Good work @harshithkashyap ! :)

@janmeier janmeier referenced this pull request Dec 21, 2016

Closed

fix for bug: Transaction name too long for MSSQL #3950 #5959

0 of 5 tasks complete

@harshithkashyap harshithkashyap deleted the harshithkashyap:mssql-tran-name-fix branch Dec 21, 2016

@mokkabonna mokkabonna referenced this pull request Oct 2, 2017

Merged

Add saveTransaction #9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment