Doctrine DBAL
  1. Doctrine DBAL
  2. DBAL-335

Is MasterSlaveConnection implemented correctly - seems to overwrite master connection on transaction methods?

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: None
    • Labels:
      None

      Description

      Forgive me to doubt, but I think there may be a bug in MasterSlaveConnection.

      It's easier to understand what I'm saying by debugging and tracing the flow, but I'll illustrate it with gists.

      First, here is a simple service method to create a user. It opens up a transaction, persists the user, commits and returns. On error, if there is an active transaction, rollback. Here is the gist:

      https://gist.github.com/3547674

      The "$conn->beginTransaction();" line is where we trace through (the remainder of the service method is now irrelevant). Looking into MasterSlaveConnection.php, we see the method tries to connect to the master connection (call this point ###):

      https://gist.github.com/3547720

      Now looking in the next gist, we see what happens when "$this->connect('master');" is called. At this point it's not that interesting, the internal "$this->_conn" property is set to "master".

      https://gist.github.com/3547750

      Now here lies the bug I believe. "parent::beginTransaction();" is called. When looking into this method, we see that another call is made to connect but this time without "master" as the argument (i.e. connect to slave). This call to connect is made before incrementing the transaction nesting level.

      https://gist.github.com/3547808

      Now, I won't do another gist for "MasterSlaveConnection::connect", but if you refer to the file at line 13 https://gist.github.com/3547750#file_master_slave_connection.php, you will see that it checks the transaction nesting level and if it is there, forces master. However, we don't increment the level until after the method returns, so the slave is used. Ultimately, this results in the internal "$this->_conn" property set to the "slave" connection which violates our original action at ### above where we said we want to connect to "master".

      Am I missing something here? Here is a gist the is a basic attempt at fixing this one method. It simply copies the code from the parent method except does not connect twice. I believe the same would have to occur for all the other methods unless it can be fixed once at the "MasterSlaveConnection::connect" level.

      https://gist.github.com/3547880

      I've just fleshed out "beginTransaction", "commit" and "rollBack" in "MasterSlaveConnection" by basically copying and pasting the code from the parent class and for my failing use case, this fixes the issue. However, it did require updating "Connection" slightly so that I had access to some private variables.

        Issue Links

          Activity

          Hide
          Lars Strojny added a comment -

          This looks indeed like a bug. From a first glimpse the fix would be to use master, if master is already connected.

          Show
          Lars Strojny added a comment - This looks indeed like a bug. From a first glimpse the fix would be to use master, if master is already connected.
          Hide
          Benjamin Eberlei added a comment -

          This only happens when "keepSlave" = true, because then the master is not written into the slave property aswell:

             } else {
               $this->connections['slave'] = $this->_conn = $this->connectTo($connectionName);
             } 
          

          Are you using keepSlave = true?

          Show
          Benjamin Eberlei added a comment - This only happens when "keepSlave" = true, because then the master is not written into the slave property aswell: } else { $ this ->connections['slave'] = $ this ->_conn = $ this ->connectTo($connectionName); } Are you using keepSlave = true?
          Hide
          Jonathan Ingram added a comment -

          Yes I am. Does that render this moot or still a bug?

          Show
          Jonathan Ingram added a comment - Yes I am. Does that render this moot or still a bug?
          Hide
          Benjamin Eberlei added a comment -

          Its still a bug, but it helps to know why this happens.

          Show
          Benjamin Eberlei added a comment - Its still a bug, but it helps to know why this happens.
          Hide
          Benjamin Eberlei added a comment -

          Fixed in master and 2.3, can you test it?

          Show
          Benjamin Eberlei added a comment - Fixed in master and 2.3, can you test it?
          Hide
          Jonathan Ingram added a comment -

          Thanks for doing this. I will test it shortly.

          Show
          Jonathan Ingram added a comment - Thanks for doing this. I will test it shortly.
          Hide
          Ivan Andric added a comment -

          Hi,

          not sure if you managed to test this but now test on mysql database fails with results below.
          Probably some typo.

          There was 1 error:

          1) Doctrine\Tests\DBAL\Functional\MasterSlaveConnectionTest::testKeepSlaveBeginTransactionStaysOnMaster
          Exception: [Doctrine\DBAL\DBALException] An exception occurred while executing 'INSERT INTO master_slave_table (test_int) VALUES ' with params

          {"1":30}

          :

          SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '30' for key 'PRIMARY'

          With queries:
          2. SQL: 'CREATE TABLE master_slave_table (test_int INT NOT NULL, PRIMARY KEY(test_int)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB' Params:

          Trace:
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:793
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285
          /home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92

          /home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DbalFunctionalTestCase.php:73
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:793
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285
          /home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92

          Caused by
          Doctrine\DBAL\DBALException: An exception occurred while executing 'INSERT INTO master_slave_table (test_int) VALUES ' with params

          {"1":30}

          :

          SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '30' for key 'PRIMARY'

          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/DBALException.php:47
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:786
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285
          /home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92

          Caused by
          PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '30' for key 'PRIMARY'

          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:786
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539
          /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285
          /home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92

          Show
          Ivan Andric added a comment - Hi, not sure if you managed to test this but now test on mysql database fails with results below. Probably some typo. There was 1 error: 1) Doctrine\Tests\DBAL\Functional\MasterSlaveConnectionTest::testKeepSlaveBeginTransactionStaysOnMaster Exception: [Doctrine\DBAL\DBALException] An exception occurred while executing 'INSERT INTO master_slave_table (test_int) VALUES ' with params {"1":30} : SQLSTATE [23000] : Integrity constraint violation: 1062 Duplicate entry '30' for key 'PRIMARY' With queries: 2. SQL: 'CREATE TABLE master_slave_table (test_int INT NOT NULL, PRIMARY KEY(test_int)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB' Params: Trace: /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:793 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285 /home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92 /home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DbalFunctionalTestCase.php:73 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:793 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285 /home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92 Caused by Doctrine\DBAL\DBALException: An exception occurred while executing 'INSERT INTO master_slave_table (test_int) VALUES ' with params {"1":30} : SQLSTATE [23000] : Integrity constraint violation: 1062 Duplicate entry '30' for key 'PRIMARY' /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/DBALException.php:47 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:786 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285 /home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92 Caused by PDOException: SQLSTATE [23000] : Integrity constraint violation: 1062 Duplicate entry '30' for key 'PRIMARY' /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:786 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539 /home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285 /home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92

            People

            • Assignee:
              Benjamin Eberlei
              Reporter:
              Jonathan Ingram
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: