added dirty read(NOLOCK) in SQLServer #4133
Conversation
| return " WITH (NOLOCK)"; | ||
|
|
||
| } else if (driver instanceof MysqlDriver || driver instanceof PostgresDriver || driver instanceof OracleDriver) { | ||
| return ""; |
There was a problem hiding this comment.
Why just ignore here? I doubt it's the default setting for those drivers.
There was a problem hiding this comment.
Hello, it's my first climb so I apologize in advance for any possible errors. Searching the internet I found this about equivalents to this instruction in other databases:
MySQL:
When WITH (nolock) is used in SQL Server to perform operation, it does not place shared lock (S) and also does not honor exclusive locks on table. The WITH (nolock) table hint is equivalent to READ UNCOMMITTED also known as "dirty read", it is the lowest level of isolation. If you specify table hint then it will override the current isolation level.
MySQL default isolation level is REPEATABLE READ which means locks will be placed for each operation but multiple connections can read data concurrently with out making table unavailable during read. MySQL database server does not support changing the default isolation mode as a hint like SQL Server.
Oracle:
Oracle by default provides a read-consistent view of data. The upshot of this design is that readers never block writers.
PostgreSQL:
PostgreSQL uses MVCC to minimize lock contention in order to allow for reasonable performance in multiuser environments. Readers do not conflict with writers nor other readers. The closest thing would be the NOWAIT
It's what I found about it.
There was a problem hiding this comment.
I still don't understand why we just pass empty sting in some drivers and not throwing LockNotSupportedOnGivenDriverError.
If driver doesn't support this it's fine but I don't think we should throw an error, not just ignore this - otherwise people using it will think that everything is working as expected while underneath typeorm would just ignore some unsupported features. I think we could omit this only if it's default behavior for those drivers.
There was a problem hiding this comment.
I think it would be better to throw in error, although really this kind of cases do not think it is an error, it would be more informative that the driver does not have that functionality. What do you think?
There was a problem hiding this comment.
I just realized that this part of the code should not be set, it is enough with the change of line 1388. I delete this part of the code.
| .where("post.id = :id", { id: 1 }) | ||
| .getSql(); | ||
|
|
||
| if (connection.driver instanceof SqlServerDriver) { |
There was a problem hiding this comment.
We limit drivers on wchich we want the test to run in two separate statements. It should be done only in one place.
There was a problem hiding this comment.
I do not know what the correction would be, any suggestions?
There was a problem hiding this comment.
What I had in mind is line 103 - we're limiting there some drivers, and here in if condition we choose only one driver after all. We could put this if condition in the beginning and there will be only one place which controls test execution based on driver.
Delete unnecesary code
|
Is everything correct or is something missing so that the merge can be done? |
|
Looks good to me. |
|
When will the changes be merged with the master branch? |
|
Looks fine, so let's merge it. |
Added support for NOLOCK in mssql
Linked to this issue
#4042