Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MishaConway
Copy link
Contributor

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
Copy link
Contributor Author

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 MishaConway force-pushed the feature-send-selects-run-via-execute-to-slaves branch from 0ebb491 to 389ca53 Compare February 15, 2016 07:08
@MishaConway
Copy link
Contributor Author

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

@MishaConway MishaConway force-pushed the feature-send-selects-run-via-execute-to-slaves branch from 389ca53 to c1206d7 Compare February 15, 2016 07:49
@sobrinho
Copy link
Collaborator

@thiagopradi what you think about that?

@sobrinho
Copy link
Collaborator

@MishaConway we need some specs :)

@MishaConway
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

MishaConway commented Jan 14, 2017

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants