octopus icon indicating copy to clipboard operation
octopus copied to clipboard

Adds support for sending queries using ActiveRecord::Base.connection.…

Open MishaConway opened this issue 9 years ago • 7 comments

Currently if you do:

sql = '....expensive sql select query.......'
result = Octopus.using(:slave_group => :slave_group_a) do
    ActiveRecord::Base.connection.execute sql
end

any available slave groups will be ignored and the select query will be run on the master instance. This feature makes slight modifications to how we determine if a select query is being run and makes the previous example attempt to send the query to any available slave groups.

The regex I am using to detect select queries will not detect every possible type of select query, but it should track the major use cases and should be a good start. Later on, we can update it so that also detects some of the more minor cases.

MishaConway avatar Feb 14 '16 08:02 MishaConway

It occurs to me that on some db drivers, it is possible to send multiple sql statements to an execute call which could be a mix of selects, writes, and deletes.

One option to protect against this is to err on the safe side with a simple failsafe and modify the regex so if the string contains a semi colon with non whitespace to the right of it, we don't send it to a slave group. The downside that we'll be missing some cases that should be sent to a slave group I think is a worthwhile tradeoff for the simplicity of this approach. Trying to be fancy here would probably require some advanced parsing to capture all cases and the likelihood of bugs would increase significantly.

MishaConway avatar Feb 14 '16 08:02 MishaConway

@sobrinho Changes mentioned in previous comment have been made and rebased back into one commit.

MishaConway avatar Feb 15 '16 07:02 MishaConway

@thiagopradi what you think about that?

sobrinho avatar Feb 19 '16 14:02 sobrinho

@MishaConway we need some specs :)

sobrinho avatar Feb 19 '16 14:02 sobrinho

@sobrinho I'll work on adding specs that test that it sends to slave group on several different cases of executes running single selects and that it doesn't whenever there is an execute running a delete or update.

MishaConway avatar Feb 19 '16 22:02 MishaConway

@thiagopradi @sobrinho Added specs!

The first verifies that if a slave group is configured and you attempt to run a simple select via ActiveRecord::Base.connection.execute that it sends the select to the slave group. As expected this test fails on the master branch, but passes in this PR.

The second verifies that non select queries like an insert via ActiveRecord::Base.connection.execute are run on master even if a slave group is currently configured.

MishaConway avatar Mar 29 '16 02:03 MishaConway

@thiagopradi @sobrinho Wondering if this can be considered again as I think it adds some important functionality that one would intuitively expect, but is missing in the current HEAD. Understand if you are not comfortable with this change. Please look at specs as a reference to what this fixes.

MishaConway avatar Jan 14 '17 20:01 MishaConway