Don't connect to write connection

[joshua-bn]

Summary (*)

In https://github.com/OpenMage/magento-lts/blob/20.0/app/code/core/Mage/Core/Model/Resource/Db/Abstract.php#L332 we are checking if the write connection has any transactions open. To do this, it connects to it. We can just check if there is a connection open first.

Examples (*)

Place a break point in Mage_Core_Model_Resource::_newConnection()

Proposed solution

Replace that with

    public function hasConnection(string $connectionName): bool
    {
        return isset($this->_connections[$connectionName]);
    }

    /**
     * Retrieve connection for read data
     *
     * @return Varien_Db_Adapter_Interface
     */
    protected function _getReadAdapter()
    {
        if ($this->hasConnection('write')) {
            $writeAdapter = $this->_getWriteAdapter();
            if ($writeAdapter && $writeAdapter->getTransactionLevel() > 0) {
                // if transaction is started we should use write connection for reading
                return $writeAdapter;
            }
        }

        return $this->_getConnection('read');
    }

I use PHP 7.4 locally and type hint so just fix it for your liking. Maybe change to protected to match the other ones, but I think this makes sense as public.

[colinmollenhour]

Excellent suggestion! I am not in favor of using type hints at this point for consistency with the rest of the code, though. Would you mind creating a PR?

[joshua-bn]

Possibly. I don't have OpenMage forked and don't actually run OpenMage. I just have been copying the relevant changes from here to our codebase (this is my work Github account - BloomNation.com).

So, if I anyone wants to take these changes and create a PR from them before I get around to it, be my guest.

I haven't actually tested this. I just found this today.

[joshua-bn]

btw, tested it locally and created the PR. I haven't tested what effect this has in production - which is really the only place you're going to see a difference in scalability and performance.

[colinmollenhour]

Thanks for creating the PR!

I wouldn't expect there to be any noticeable impact on performance even if you've disabled the ill-conceived Mage_Log module and are using a replica for read connections, but it still makes sense not to open connections unless/until they are going to actually be used. I think for the out of the box installation the read/write connections are still shared anyway so for those installations there shouldn't be any difference.

[joshua-bn]

Well, even the OOTB install will make two connections to the db since it doesn't realize it's the same connection. MySQL connections are relatively fast - when compared to other databases and the queries themselves - but they still have a cost. On my local the db is in a Docker container on the same computer so it's going to be almost nothing. If you have a db with some physical distance and a networking distance, that could be a lot more.

By no means do I think this accounts for anything noticeable though. I think it might help with some load on the master for some (very few) people.

[tmotyl]

Hi
Does it save a connection per query or a connection per HTTP request?

This might become helpful/ important more from stability point then performance (being able to handle bigger load with lower connection limit on mysql).

[joshua-bn]

It caches the connection per request. Right now you'll have two connections for every request. With this, most requests will have one and two when you actually need to write something to the db.

It's a bit of scalability and a bit of performance. Neither are going to make or break the application as a whole, but maybe help a little.

[colinmollenhour]

There is only one connection per request unless you actually define separate resources in your config:

[joshua-bn]

Ah okay, I have default_setup and default_read in my local.xml, even for my local dev env.