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

[MCC-833873] Fix single pg call for accessible_objects from replica #192

Merged
merged 5 commits into from
Nov 16, 2021

Conversation

kjerinsky-mdsol
Copy link
Contributor

@kjerinsky-mdsol kjerinsky-mdsol commented Nov 15, 2021

PostgreSQL has restrictions on creating temporary tables on replica databases. Adding in the CTE that was used in previous benchmark investigations for use when the database connection a replica.

Benchmarks for 50k user

Single run

method version user system total real faster
no pluck 3.163534 0.151665 3.315199 3.845629
pluck 1.610845 0.035368 1.646213 2.271648 40.1%
pg function 0.020938 0.012064 0.033002 0.452649 88.2%
cte 0.041312 0.007820 0.049132 0.624768 83.8%

Multiple runs (5)

method version user system total real faster
no pluck 17.658298 0.479222 18.137520 21.150227
pluck 7.143887 0.198251 7.342138 10.431428 50.7%
pg function 0.182273 0.055765 0.238038 2.381902 88.7%
cte 0.163428 0.039802 0.203230 3.082762 85.4%

@kjerinsky-mdsol kjerinsky-mdsol self-assigned this Nov 15, 2021
@cmcinnes-mdsol
Copy link
Contributor

@kjerinsky-mdsol we also need to fix the loading issue that leads to rake tasks failing and containers failing when they first spin up

@@ -33,5 +33,5 @@ Gem::Specification.new do |s|
s.add_development_dependency('byebug')
s.add_development_dependency('neography', '~> 1.1')
s.add_development_dependency('database_cleaner')
s.add_development_dependency('rails', '~> 6.0')
s.add_development_dependency('rails', '~> 6.1')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the bump? asking cause im wondering if this is indeed a development dependency or actually a runtime dependency

Copy link
Contributor Author

@kjerinsky-mdsol kjerinsky-mdsol Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because ActiveRecord::Base.connection_db_config.config is logging from Rails 6.1

DEPRECATION WARNING: DatabaseConfig#config will be removed in 6.2.0 in favor of DatabaseConfigurations#configuration_hash which returns a hash with symbol keys (called from irb_binding at (irb):20)

Rails 6.0 doesn't have DatabaseConfigurations#configuration_hash so either get ahead of it or let it potentially sneak up on us.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That or check for either method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is mainly for the test Rails app that is created for running specs I think. If we really cared about this gem we would make it use appraisals for various Ruby/Rails combinations... but, to me, just using the same version as Dalto is fine.

@@ -33,5 +33,5 @@ Gem::Specification.new do |s|
s.add_development_dependency('byebug')
s.add_development_dependency('neography', '~> 1.1')
s.add_development_dependency('database_cleaner')
s.add_development_dependency('rails', '~> 6.0')
s.add_development_dependency('rails', '~> 6.1')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comment

end

def self.replica?
::ActiveRecord::Base.connection_db_config.configuration_hash[:replica] == true
Copy link
Contributor

@ejinotti-mdsol ejinotti-mdsol Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a function for doing some ridiculous rigmarole to handle the ever-changing ActiveRecord API surface for getting a DB config:

def self.db_config
return @_db_config if @_db_config
ar_configs = PolicyElement.configurations
# ActiveRecord < 6.0
unless ar_configs.respond_to?(:configs_for)
@_db_config = ar_configs[Rails.env].symbolize_keys
return @_db_config
end
config = begin
# ActiveRecord >= 6.1:
# there is a deprecation warning for using kwarg 'spec_name'
# so try the new 'name' kwarg first
ar_configs.configs_for(env_name: Rails.env, name: 'primary')
rescue ArgumentError
# ActiveRecord == 6.0:
# the kwarg is called 'spec_name'
ar_configs.configs_for(env_name: Rails.env, spec_name: 'primary')
end
@_db_config =
# AR 6.1 also emits a warning that `.config` is deprecated
# and `.configuration_hash` is The New Way
if config.respond_to?(:configuration_hash)
config.configuration_hash
else
config.config.symbolize_keys
end
end

Should probably use it somehow.

@@ -33,5 +33,5 @@ Gem::Specification.new do |s|
s.add_development_dependency('byebug')
s.add_development_dependency('neography', '~> 1.1')
s.add_development_dependency('database_cleaner')
s.add_development_dependency('rails', '~> 6.0')
s.add_development_dependency('rails', '~> 6.1')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is mainly for the test Rails app that is created for running specs I think. If we really cared about this gem we would make it use appraisals for various Ruby/Rails combinations... but, to me, just using the same version as Dalto is fine.

@cmcinnes-mdsol cmcinnes-mdsol merged commit 4f234bd into develop Nov 16, 2021
@cmcinnes-mdsol cmcinnes-mdsol deleted the feature/accessible-objects-for-replica branch November 16, 2021 16:30
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.

3 participants