Fix dropping in use databases on SQL Server and Oracle #2636

deeky666 · 2017-02-06T02:32:54Z

This patch follows the same approach as for PostgreSQL. If trying to drop a database with active connections, the database server will refuse with:

SQLSTATE [42000, 3702]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Cannot drop database "%s" because it is currently in use.

The schema manager catches that specific error code now, closes all active connections on that database and executes a DROP DATABASE again.

I had to fix the SQLSrvException to carry around the SQLSTATE and error code information.

Ocramius · 2017-02-06T10:33:08Z

lib/Doctrine/DBAL/Schema/SQLServerSchemaManager.php

+                throw $exception;
+            }
+
+            $this->_execSql($this->_platform->getCloseActiveDatabaseConnectionsSQL($database));


Call on a method not defined on the abstract class

Same as with PostgreSQL. What shall I do here?

@deeky666 one possible solution is to turn the return type into a string[], then have a base method always returning an empty array. array_walk-ing over the result is sufficient to ensure consistency then.

Ocramius · 2017-02-06T10:33:25Z

lib/Doctrine/DBAL/Schema/SQLServerSchemaManager.php

+            // because of active connections on the database.
+            // To force dropping the database, we first have to close all active connections
+            // on that database and issue the drop database operation again.
+            if ($exception->getErrorCode() !== 3702) {


Move it to a constant, please (and document the constant)

I disagree here. It doesn't help anyone and if we start to introduce constants for all error codes and SQL states, we will go crazy. We started with this approach when we implemented driver exception converters and discarded that approach again. This check will disappear as soon as we implement SQL Server driver as driver exception converter drivers (hopefully) as we will be able to catch a dedicated exception type then. Also it is implemented that way for PostgreSQL already and I don't want to do everything that is necessary to make it "perfect" with this PR. First I want to get the SQL Server test suite to be green. We can still improve afterwards. What do you think?

Works for me, since there is inline documentation. Also, didn't expect you to fix every instance in here.

If/when we move to PHP 7.1, then we should consider private constants.

Ocramius · 2017-02-06T10:34:14Z

lib/Doctrine/DBAL/Schema/SQLServerSchemaManager.php

@@ -37,6 +37,31 @@ class SQLServerSchemaManager extends AbstractSchemaManager
    /**
     * {@inheritdoc}
     */
+    public function dropDatabase($database)


The code additions here are untested. Can the scenario be replicated?

There is already a test case that covers this issue in SchemaManagerFunctionalTestCase::testDropsDatabaseWithActiveConnections()

Ocramius · 2017-02-06T10:34:41Z

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php

+     *
+     * @return string
+     */
+    public function getCloseActiveDatabaseConnectionsSQL($database)


This method should probably land in the parent classes too then (sadly) :-\

I get your point but I do not want to enforce an abstraction here because it probably will be too vendor specific. Another option would be to put the SQL directly into the dropDatabase() method in the schema manager.

Possibly a better solution if the schema manager is platform specific (and won't rely on the platform abstraction anyway).

So this is a "yes" let's put the SQL directly into the schema manager?

Yes, let's stuff it in the schema manager, and add a comment about the decision

Ocramius · 2017-02-06T10:34:55Z

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php

+     */
+    public function getCloseActiveDatabaseConnectionsSQL($database)
+    {
+        return 'ALTER DATABASE ' . $database . ' SET SINGLE_USER WITH ROLLBACK IMMEDIATE';


Should $database be quoted?

Using Identifier now.

deeky666 · 2017-02-07T18:06:18Z

@Ocramius marking this as WIP for now as I still need to figure out how to make nasty Oracle pass here. I am testing against version 12 locally and there seem to be some difference to version 11 (CI).

deeky666 · 2017-02-07T20:15:12Z

Documented an issue revealed while testing with Orcale 12c in #2643
The issue is unrelated and should not be fixed here.

deeky666 · 2017-02-07T20:44:23Z

@Ocramius ok patch is finished from my side now. You can review again. Some remarks:

Obviously I had to fix the exact same issue on Oracle to make the tests pass. Approach is the same. I also had to change the privileges when creating a new database (user) on Oracle so that the user is able to see the views used in killUserSessions() when force dropping a database. That should not be an issue though, as users created by DBAL are not used as users but as databases (that is still confusing after all). Please blame Oracle. Thanks. BYEEEEE!

Ocramius · 2017-02-08T08:37:28Z

LGTM! 👍

…nd-oracledb' into 2.5 Backport #2636 to 2.5.x

Ocramius · 2017-02-08T08:40:52Z

Backported into 2.5 via c13ee0c

deeky666 added Bug Databases Platforms Schema Management SQL Server labels Feb 6, 2017

deeky666 requested a review from Ocramius February 6, 2017 02:32

deeky666 mentioned this pull request Feb 6, 2017

Testing of MSSQL on Windows with AppVeyor #2617

Merged

Ocramius suggested changes Feb 6, 2017

View reviewed changes

deeky666 force-pushed the fix-sqlserver-drop-inuse-database branch from 85a2d60 to 10a12b5 Compare February 6, 2017 18:42

Ocramius assigned deeky666 Feb 7, 2017

deeky666 force-pushed the fix-sqlserver-drop-inuse-database branch 2 times, most recently from 79b449f to 7b631e7 Compare February 7, 2017 15:31

deeky666 changed the title ~~Fix dropping inuse databases on SQL Server~~ [WIP] Fix dropping inuse databases on SQL Server Feb 7, 2017

deeky666 added 3 commits February 7, 2017 21:18

fix drop inuse database functional test

864a78f

fix dropping inuse databases on SQL Server

f9384ae

fix dropping inuse databases on Oracle

ef736a5

deeky666 force-pushed the fix-sqlserver-drop-inuse-database branch from ece24a2 to ef736a5 Compare February 7, 2017 20:26

deeky666 changed the title ~~[WIP] Fix dropping inuse databases on SQL Server~~ [WIP] Fix dropping inuse databases on SQL Server ans Oracle Feb 7, 2017

deeky666 changed the title ~~[WIP] Fix dropping inuse databases on SQL Server ans Oracle~~ [WIP] Fix dropping inuse databases on SQL Server and Oracle Feb 7, 2017

deeky666 changed the title ~~[WIP] Fix dropping inuse databases on SQL Server and Oracle~~ Fix dropping inuse databases on SQL Server and Oracle Feb 7, 2017

Ocramius approved these changes Feb 8, 2017

View reviewed changes

Ocramius added this to the 2.5.12 milestone Feb 8, 2017

Ocramius merged commit 7e0a6cc into doctrine:master Feb 8, 2017

Ocramius changed the title ~~Fix dropping inuse databases on SQL Server and Oracle~~ Fix dropping in use databases on SQL Server and Oracle Feb 8, 2017

Ocramius added a commit that referenced this pull request Feb 8, 2017

Merge branch 'fix/#2636-fix-dropping-in-use-databases-on-sql-server-a…

c13ee0c

…nd-oracledb' into 2.5 Backport #2636 to 2.5.x

greg0ire mentioned this pull request Aug 25, 2020

Sporadic test failure on PDO_OCI #4225

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dropping in use databases on SQL Server and Oracle #2636

Fix dropping in use databases on SQL Server and Oracle #2636

deeky666 commented Feb 6, 2017

Ocramius Feb 6, 2017

deeky666 Feb 6, 2017

Ocramius Feb 7, 2017

Ocramius Feb 6, 2017

deeky666 Feb 6, 2017

Ocramius Feb 7, 2017

deeky666 Feb 7, 2017

Ocramius Feb 6, 2017

deeky666 Feb 6, 2017

Ocramius Feb 6, 2017

deeky666 Feb 6, 2017

Ocramius Feb 7, 2017

deeky666 Feb 7, 2017

Ocramius Feb 7, 2017

deeky666 Feb 7, 2017

Ocramius Feb 6, 2017

deeky666 Feb 6, 2017

deeky666 commented Feb 7, 2017

deeky666 commented Feb 7, 2017

deeky666 commented Feb 7, 2017

Ocramius commented Feb 8, 2017

Ocramius commented Feb 8, 2017

Fix dropping in use databases on SQL Server and Oracle #2636

Fix dropping in use databases on SQL Server and Oracle #2636

Conversation

deeky666 commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deeky666 commented Feb 7, 2017

deeky666 commented Feb 7, 2017

deeky666 commented Feb 7, 2017

Ocramius commented Feb 8, 2017

Ocramius commented Feb 8, 2017