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

Part 7: Multi db improvements, Add ability to block writes to a database #34505

Merged
merged 1 commit into from Nov 30, 2018

Conversation

@eileencodes
Copy link
Member

@eileencodes eileencodes commented Nov 21, 2018

We use this at GH but I'm not 100% sure whether others will find this useful. Thoughts?

The other open question I have is whether the write_query? should be defined by "is a query that writes" or "is a query that is not reading" (ie not select, etc). For now I chose some common write queries to demonstrate the goals of this feature but I'd love input into what @rafaelfranca @tenderlove and @matthewd think about this.


This PR adds the ability to block writes to a database even if the
database user is able to write (ie the database is a primary and not a
replica).

This is useful for a few reasons: 1) when converting your database from
a single db to a primary/replica setup - you can fix all the writes on
reads early on, 2) when we implement automatic database switching or
when an app is manually switching connections this feature can be used
to ensure reads are reading and writes are writing. We want to make sure
we raise if we ever try to write in read mode, regardless of database
type.

This should be used in conjunction with connected_to in write mode.
For example:

ActiveRecord::Base.connected_to(role: :writing) do
  Dog.connection.while_blocking_writes do
    Dog.create! # will raise because we're blocking writes
  end
end

ActiveRecord::Base.connected_to(role: :reading) do
  Dog.connection.while_blocking_writes do
    Dog.first # will not raise because we're not writing
  end
end
@eileencodes eileencodes added this to the 6.0.0 milestone Nov 21, 2018
@eileencodes eileencodes self-assigned this Nov 21, 2018
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Nov 21, 2018

We do have code to do the same thing. So this would be useful for us.

We define it as "is a query that is not reading" (ie not select, etc)", and we use:

      build_union_regexp = ->(*parts) do
        parts = parts.map { |part| /\A\s*#{part}/i }
        Regexp.union(*parts)
      end
      READ_QUERY = build_union_regexp.call(:select, :show, :set)

as definition.

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 21, 2018

I like the idea of it, because it'd provide a good foundation to run a read/write-split application against a single database in development.

I don't think it's practical to detect read-only queries via string matching, though: even something that explicitly starts with "SELECT" can end up writing. I wonder about using SET SESSION TRANSACTION READ ONLY to have the DB enforce it instead? (Also worth noting I believe most write-ish queries go via execute, not exec_query.)

On a specific API note, I'd prefer a synonym for 'blocking', just because it conflicts with "[thread] blocking I/O".

@eileencodes eileencodes force-pushed the eileencodes:add-readonly-mode branch 2 times, most recently from 775c0ca to 132baad Nov 27, 2018
@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented Nov 27, 2018

Changed while_blocking_writes to while_preventing_writes to address @matthewd's concern. I also updated the SQL matcher to use the regex from @rafaelfranca and added some more tests.

@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented Nov 27, 2018

Oh right I also need to move this to execute. 😄

@paracycle
Copy link
Contributor

@paracycle paracycle commented Nov 27, 2018

I agree with @matthewd that detecting read-only queries via string matching might be problematic. For example SELECT ... INTO is definitely not a read-only query but would pass the check. I am not sure what the best alternative is, though, but making the DB enforce it sounds much better.

@eileencodes eileencodes force-pushed the eileencodes:add-readonly-mode branch 3 times, most recently from f313a51 to 392d51e Nov 29, 2018
@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented Nov 29, 2018

Ok I moved this to execute and added tests for create/delete/update/where.first to ensure these were all getting caught.

I updated the changelog to note that the purpose of preventing write queries when choosing readonly mode is for testing, catching accidental writes, and for switching to a readonly connection without opening a second connection. It's purpose isn't to catch ALL writes to a database - it's not meant as a replacement for a replica.

This PR adds the ability to prevent writes to a database even if the
database user is able to write (ie the database is a primary and not a
replica).

This is useful for a few reasons: 1) when converting your database from
a single db to a primary/replica setup - you can fix all the writes on
reads early on, 2) when we implement automatic database switching or
when an app is manually switching connections this feature can be used
to ensure reads are reading and writes are writing. We want to make sure
we raise if we ever try to write in read mode, regardless of database
type and 3) for local development if you don't want to set up multiple
databases but do want to support rw/ro queries.

This should be used in conjunction with `connected_to` in write mode.
For example:

```
ActiveRecord::Base.connected_to(role: :writing) do
  Dog.connection.while_preventing_writes do
    Dog.create! # will raise because we're preventing writes
  end
end

ActiveRecord::Base.connected_to(role: :reading) do
  Dog.connection.while_preventing_writes do
    Dog.first # will not raise because we're not writing
  end
end
```
@eileencodes eileencodes force-pushed the eileencodes:add-readonly-mode branch from 392d51e to f39d72d Nov 30, 2018
@eileencodes eileencodes merged commit f6e1061 into rails:master Nov 30, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eileencodes eileencodes deleted the eileencodes:add-readonly-mode branch Nov 30, 2018
kamipo added a commit that referenced this pull request Dec 10, 2018
Follow up #34505.
@eileencodes eileencodes changed the title Add ability to block writes to a database Part 7: Multi db improvements, Add ability to block writes to a database Jan 30, 2019
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jul 1, 2019
@Fivell
Copy link
Contributor

@Fivell Fivell commented May 6, 2020

@eileencodes what about copy to CSV ? in this implementation it is going to be writable but it isn't

@@ -67,11 +67,21 @@ def query(sql, name = nil) #:nodoc:
end
end

READ_QUERY = ActiveRecord::ConnectionAdapters::AbstractAdapter.build_read_query_regexp.call(:select, :show, :set) # :nodoc:

This comment has been minimized.

@bogdan

bogdan May 6, 2020
Contributor

explain ?

This comment has been minimized.

@eileencodes

eileencodes May 6, 2020
Author Member

Explain is on master.

DEFAULT_READ_QUERY = [:begin, :commit, :explain, :release, :rollback, :savepoint, :select, :with] # :nodoc:

@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented May 6, 2020

what about copy to CSV

This feature is not meant to be a catch all. If you want to actually block all writes you should use a readonly user.

@Fivell
Copy link
Contributor

@Fivell Fivell commented Aug 10, 2020

@eileencodes I don't want to block writes, just expect using copy to csv will not raise error created issue #39166

@janko
Copy link
Contributor

@janko janko commented Nov 4, 2020

I'm curious, why was this approach preferred over read-only transactions, which @matthewd had mentioned in #34505 (comment)? Extending Active Record transactions to support a new option e.g. :read_only that would set READ ONLY access mode seems much simpler, and also more correct because it would be leveraging the database instead of regex-matching SQL strings.

@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented Nov 4, 2020

I answered @janko here https://discuss.rubyonrails.org/t/active-record-while-preventing-writes-api-versus-read-only-transactions/76443/2.

Can we keep the discussion in one place if there are followup questions? Thanks!

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

Successfully merging this pull request may close these issues.

None yet

7 participants