fix SQLServerPlatform locking hints #257

Merged
merged 4 commits into from Feb 3, 2013

Conversation

Projects
None yet
7 participants
Member

deeky666 commented Jan 25, 2013

This should completely implement the locking hints of the SQLServerPlatform according to http://msdn.microsoft.com/en-us/library/aa213026%28v=sql.80%29.aspx.

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DBAL-427

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
@@ -845,16 +846,19 @@ public function rollbackSavePoint($savepoint)
*/
public function appendLockHint($fromClause, $lockMode)
{
- // @todo correct
+ $tableHint = '';
+
+ if ($lockMode == \Doctrine\DBAL\LockMode::NONE) {
+ $tableHint = ' WITH (READUNCOMMITTED)'; // equivalent to NOLOCK
+ }
if ($lockMode == \Doctrine\DBAL\LockMode::PESSIMISTIC_READ) {
@Ocramius

Ocramius Jan 26, 2013

Owner

Our CS requires to have newlines between if blocks

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
}
-
@Ocramius

Ocramius Jan 26, 2013

Owner

As above. Don't remove this line

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
}
- return $fromClause;
+ return $fromClause . $tableHint;
@Ocramius

Ocramius Jan 26, 2013

Owner

I think that 3 return statements is better than this solution

@stof stof commented on an outdated diff Jan 26, 2013

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
@@ -845,16 +846,19 @@ public function rollbackSavePoint($savepoint)
*/
public function appendLockHint($fromClause, $lockMode)
{
- // @todo correct
+ $tableHint = '';
+
+ if ($lockMode == \Doctrine\DBAL\LockMode::NONE) {
@stof

stof Jan 26, 2013

Member

you should add a use statement IMO

Owner

beberlei commented Jan 30, 2013

"Should" seems very unsure, have you actually tested this?

@guilhermeblanco guilhermeblanco commented on an outdated diff Feb 3, 2013

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
@@ -845,13 +847,16 @@ public function rollbackSavePoint($savepoint)
*/
public function appendLockHint($fromClause, $lockMode)
{
- // @todo correct
- if ($lockMode == \Doctrine\DBAL\LockMode::PESSIMISTIC_READ) {
- return $fromClause . ' WITH (tablockx)';
+ if ($lockMode == LockMode::NONE) {
@guilhermeblanco

guilhermeblanco Feb 3, 2013

Owner

This should become a switch. Something like this:

    public function appendLockHint($fromClause, $lockMode)
    {
        switch ($lockMode) {
            case LockMode::PESSIMISTIC_READ:
                $lockClause = ' WITH (READCOMMITTED)';
                break;

            case LockMode::PESSIMISTIC_WRITE:
                $lockClause = ' WITH (SERIALIZABLE)';
                break;

            case LockMode::NONE:
                $lockClause = ' WITH (READUNCOMMITTED)';
                break;

            default:
                $lockClause = '';
        }

        return $fromClause . $lockClause;
    }
Member

deeky666 commented Feb 3, 2013

Ok I tested this now on a SQL Server database and fixed the locking hints again. I mixed up something with pessimistic read and pessimistic write before. This is also how hibernate implements the locking hints.

@guilhermeblanco guilhermeblanco added a commit that referenced this pull request Feb 3, 2013

@guilhermeblanco guilhermeblanco Merge pull request #257 from deeky666/fix-sqlserverplatform-lockhints
fix SQLServerPlatform locking hints
02ea5f0

@guilhermeblanco guilhermeblanco merged commit 02ea5f0 into doctrine:master Feb 3, 2013

1 check passed

default The Travis build passed
Details

This change highlighted a bug in the ORM SqlWalker -- #DDC-2310

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