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

Add prepared statements support for `Mysql2Adapter` #23461

Merged
merged 1 commit into from Apr 24, 2016

Conversation

Projects
None yet
8 participants
@kamipo
Member

kamipo commented Feb 3, 2016

Reopen #22415.

Currently mysql2 adapter does not support prepared statements. And mysql adapter has been removed. This means that Active Record does not support prepared statements for MySQL now.

We need to support prepared statements for MySQL. But mysql2 gem still have GC issue in prepared statements. brianmario/mysql2#694

I made default to @prepared_statements = false in Mysql2Adapter for does not affect default behavior. I think it is still safety.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Feb 3, 2016

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

rails-bot commented Feb 3, 2016

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

@sgrif

View changes

Show outdated Hide outdated ...verecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Feb 4, 2016

Member

Extracted unrelated change to #23469, thanks!
r? @sgrif

Member

kamipo commented Feb 4, 2016

Extracted unrelated change to #23469, thanks!
r? @sgrif

@rails-bot rails-bot assigned sgrif and unassigned chancancode Feb 4, 2016

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Feb 5, 2016

Member

This will need to handle nodes which we consider to be unpreparable, similar to what we do in PG and SQLite. We can probably just move that logic up to the abstract adapter now.

Member

sgrif commented Feb 5, 2016

This will need to handle nodes which we consider to be unpreparable, similar to what we do in PG and SQLite. We can probably just move that logic up to the abstract adapter now.

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Feb 6, 2016

Member

Do you means #23515?

Member

kamipo commented Feb 6, 2016

Do you means #23515?

@sodabrew

This comment has been minimized.

Show comment
Hide comment
@sodabrew

sodabrew Feb 24, 2016

Contributor

I've just pushed mysql2 0.4.3 with important prepared statement fixes from @kamipo!

Contributor

sodabrew commented Feb 24, 2016

I've just pushed mysql2 0.4.3 with important prepared statement fixes from @kamipo!

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Apr 19, 2016

Member

Rebase time 😊

Member

jeremy commented Apr 19, 2016

Rebase time 😊

@kamipo

View changes

Show outdated Hide outdated activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
@sodabrew

This comment has been minimized.

Show comment
Hide comment
@sodabrew

sodabrew Apr 19, 2016

Contributor

I've just posted mysql2 0.4.4 with additional prepared statement fixes from @kamipo!

Contributor

sodabrew commented Apr 19, 2016

I've just posted mysql2 0.4.4 with additional prepared statement fixes from @kamipo!

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Apr 19, 2016

Member

Thanks @sodabrew !! 😍

Member

kamipo commented Apr 19, 2016

Thanks @sodabrew !! 😍

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Apr 19, 2016

Member

Rebased on master!

Member

kamipo commented Apr 19, 2016

Rebased on master!

# Clears the prepared statements cache.
def clear_cache!
reload_type_map

@jeremy jeremy added this to the 5.1.0 milestone Apr 20, 2016

def initialize(connection, logger, connection_options, config)
super
@prepared_statements = false
@prepared_statements = false unless config.key?(:prepared_statements)

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

Should we use the value of the prepared_statements config here like we do in the other adapters?

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

Should we use the value of the prepared_statements config here like we do in the other adapters?

This comment has been minimized.

@kamipo

kamipo Apr 20, 2016

Member

This is for prepared_statements disabled by default (keep previous default).
Should we make to enabled by default?

@kamipo

kamipo Apr 20, 2016

Member

This is for prepared_statements disabled by default (keep previous default).
Should we make to enabled by default?

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

We should respect it. If users ask to enable it, it should be enabled, to disable it, it should be disabled. This implementation while keeping the default is enabling if you do prepared_statements: false in your config.

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

We should respect it. If users ask to enable it, it should be enabled, to disable it, it should be disabled. This implementation while keeping the default is enabling if you do prepared_statements: false in your config.

This comment has been minimized.

@kamipo

kamipo Apr 20, 2016

Member

This implementation is:

if config.key?(:prepared_statements)
  # @prepared_statements is configured in `super`
  super
else
  # keep the previous default (disabled) when `:prepared_statements` is not specified
  @prepared_statements = false
end
@kamipo

kamipo Apr 20, 2016

Member

This implementation is:

if config.key?(:prepared_statements)
  # @prepared_statements is configured in `super`
  super
else
  # keep the previous default (disabled) when `:prepared_statements` is not specified
  @prepared_statements = false
end

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

Oh. I missed that change that moved AbstractAdapter to configure prepared_statements 👍

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

Oh. I missed that change that moved AbstractAdapter to configure prepared_statements 👍

# as values.
def select_one(arel, name = nil, binds = [])
arel, binds = binds_from_relation(arel, binds)
@connection.query_options.merge!(as: :hash)

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

What this does?

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

What this does?

This comment has been minimized.

@kamipo

kamipo Apr 20, 2016

Member

stmt.execute calls result.each in the internal.
https://github.com/brianmario/mysql2/blob/0.4.4/ext/mysql2/statement.c#L384
Need to set query_options before stmt.execute.

@kamipo

kamipo Apr 20, 2016

Member

stmt.execute calls result.each in the internal.
https://github.com/brianmario/mysql2/blob/0.4.4/ext/mysql2/statement.c#L384
Need to set query_options before stmt.execute.

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

But how returning a hash instead of array in the result would be affected by the result.each call?

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

But how returning a hash instead of array in the result would be affected by the result.each call?

This comment has been minimized.

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

I see now. Thanks for the explanation!

@rafaelfranca

rafaelfranca Apr 20, 2016

Member

I see now. Thanks for the explanation!

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Apr 20, 2016

Member

Changing to 5.0 since prepared statements are disabled by default.

Member

jeremy commented Apr 20, 2016

Changing to 5.0 since prepared statements are disabled by default.

@jeremy jeremy modified the milestones: 5.0.0, 5.1.0 Apr 20, 2016

@rafaelfranca

View changes

Show outdated Hide outdated ...verecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
gem 'mysql2', '>= 0.3.18', '< 0.5'
gem 'mysql2', '~> 0.4.4'

This comment has been minimized.

@jeremy

jeremy Apr 24, 2016

Member

Do we need to bump mysql2 minimum version? 0.4.x is only required if prepared_statements: true.

@jeremy

jeremy Apr 24, 2016

Member

Do we need to bump mysql2 minimum version? 0.4.x is only required if prepared_statements: true.

This comment has been minimized.

@kamipo

kamipo Apr 24, 2016

Member

Unfortunately, mysql2 0.4.3 was broken without prepared statements.
I would like to skip this version.

@kamipo

kamipo Apr 24, 2016

Member

Unfortunately, mysql2 0.4.3 was broken without prepared statements.
I would like to skip this version.

@jeremy jeremy merged commit 3f6574e into rails:master Apr 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jeremy added a commit that referenced this pull request Apr 24, 2016

Merge pull request #23461 from kamipo/prepared_statements_for_mysql2_…
…adapter

Add prepared statements support for `Mysql2Adapter`

@kamipo kamipo deleted the kamipo:prepared_statements_for_mysql2_adapter branch Apr 24, 2016

vipulnsward added a commit to vipulnsward/rails that referenced this pull request Apr 24, 2016

kaspth added a commit that referenced this pull request Apr 24, 2016

@sodabrew

This comment has been minimized.

Show comment
Hide comment
@sodabrew

sodabrew Apr 24, 2016

Contributor

Hurray!

Contributor

sodabrew commented Apr 24, 2016

Hurray!

vipulnsward added a commit to vipulnsward/rails that referenced this pull request Apr 24, 2016

Follow up of #23461
- Rename max to statement_limit
- Remove magic number 1000 from everywhere
- Defined StatementPool::DEFAULT_STATEMENT_LIMIT and started using it everywhere

jeremy added a commit that referenced this pull request Apr 24, 2016

Follow up of #23461
- Rename max to statement_limit
- Remove magic number 1000 from everywhere
- Defined StatementPool::DEFAULT_STATEMENT_LIMIT and started using it everywhere

Signed-off-by: Jeremy Daer <jeremydaer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment