From e98df01a9aa82f7daa234feda0822e26aa0de29e Mon Sep 17 00:00:00 2001 From: Ethan Donowitz <134718600+ethan-readyset@users.noreply.github.com> Date: Fri, 19 Jan 2024 16:43:04 -0500 Subject: [PATCH] rake: Fix migration tasks (#71) Previously, the migration task relied upon the query IDs generated by ReadySet to determine what caches it needed to create or drop. This was error prone because when caches are created via the gem, they are created via the ID generated from the query string generated by Rails when it invoked the queries. When the caches are created via the *migration file*, they are created via the query string that was obtained from calling `SHOW CACHES`, which has a number of rewrites, including star expansion and fully-qualified table and column names. Creating a cache from this query string would create a cache with a different ID than creating the same cache from the Rails-generated query string. In addition, the migration task was always attempting to create caches via the query ID from the migration file, which relies upon there being an entry in SHOW PROXIED QUERIES for that query ID. This may not always be the case, since it's possible a user may run the cache migrations before routing queries for those caches. This commit fixes these two issues by removing the dependence on query IDs entirely and instead compares the query strings in the migration file to the query strings in SHOW CACHES to determine which caches should be created and dropped. Finally, this commit also rewrites the rake task tests to run directly against the Postgres and ReadySet instances running in CI to ensure that the logic is sound. --- lib/readyset/caches.rb | 3 +- lib/tasks/readyset.rake | 37 +++++++++++------------- lib/templates/caches.rb.tt | 2 +- spec/caches_spec.rb | 12 ++++---- spec/rake_spec.rb | 59 ++++++++++++++++++-------------------- 5 files changed, 53 insertions(+), 60 deletions(-) diff --git a/lib/readyset/caches.rb b/lib/readyset/caches.rb index 94fef96..5323266 100644 --- a/lib/readyset/caches.rb +++ b/lib/readyset/caches.rb @@ -6,13 +6,12 @@ class << self attr_reader :caches end - def self.cache(id:, always: false) + def self.cache(always: false) @caches ||= Set.new query = yield @caches << Query::CachedQuery.new( - id: id, text: query.strip, always: always, ) diff --git a/lib/tasks/readyset.rake b/lib/tasks/readyset.rake index bdf110f..ca22eb3 100644 --- a/lib/tasks/readyset.rake +++ b/lib/tasks/readyset.rake @@ -11,6 +11,7 @@ namespace :readyset do template = File.read(File.join(File.dirname(__FILE__), '../templates/caches.rb.tt')) queries = Readyset::Query::CachedQuery.all + f = File.new(Readyset.configuration.migration_path, 'w') f.write(ERB.new(template, trim_mode: '-').result(binding)) f.close @@ -29,41 +30,37 @@ namespace :readyset do # definition of the `Readyset::Caches` subclass is garbage collected too container = Object.new container.instance_eval(File.read(file)) - caches = container.singleton_class::ReadysetCaches.caches - - caches_on_readyset = Readyset::Query::CachedQuery.all.index_by(&:id) - caches_on_readyset_ids = caches_on_readyset.keys.to_set - - caches_in_migration_file = caches.index_by(&:id) - caches_in_migration_file_ids = caches_in_migration_file.keys.to_set + caches_in_migration_file = container.singleton_class::ReadysetCaches.caches.index_by(&:text) + caches_on_readyset = Readyset::Query::CachedQuery.all.index_by(&:text) - to_drop_ids = caches_on_readyset_ids - caches_in_migration_file_ids - to_create_ids = caches_in_migration_file_ids - caches_on_readyset_ids + to_drop = caches_on_readyset.keys - caches_in_migration_file.keys + to_create = caches_in_migration_file.keys - caches_on_readyset.keys - if to_drop_ids.size.positive? || to_create_ids.size.positive? + if to_drop.size.positive? || to_create.size.positive? dropping = 'Dropping'.red creating = 'creating'.green - print "#{dropping} #{to_drop_ids.size} caches and #{creating} #{to_create_ids.size} " \ - 'caches. Continue? (y/n) ' + print "#{dropping} #{to_drop.size} caches and #{creating} #{to_create.size} caches. " \ + 'Continue? (y/n) ' $stdout.flush y_or_n = STDIN.gets.strip if y_or_n == 'y' - if to_drop_ids.size.positive? - bar = ProgressBar.create(title: 'Dropping caches', total: to_drop_ids.size) + if to_drop.size.positive? + bar = ProgressBar.create(title: 'Dropping caches', total: to_drop.size) - to_drop_ids.each do |id| + to_drop.each do |text| bar.increment - Readyset.drop_cache!(name_or_id: id) + Readyset.drop_cache!(caches_on_readyset[text].name) end end - if to_create_ids.size.positive? - bar = ProgressBar.create(title: 'Creating caches', total: to_create_ids.size) + if to_create.size.positive? + bar = ProgressBar.create(title: 'Creating caches', total: to_create.size) - to_create_ids.each do |id| + to_create.each do |text| bar.increment - Readyset.create_cache!(id: id) + cache = caches_in_migration_file[text] + Readyset.create_cache!(sql: text, always: cache.always) end end end diff --git a/lib/templates/caches.rb.tt b/lib/templates/caches.rb.tt index 78a8c13..96d61fd 100644 --- a/lib/templates/caches.rb.tt +++ b/lib/templates/caches.rb.tt @@ -1,6 +1,6 @@ class ReadysetCaches < Readyset::Caches <% queries.each do |query| -%> - cache id: <%= query.id.dump %>, always: <%= query.always %> do + cache always: <%= query.always %> do <<~SQL <%= query.text.gsub("\n", "\n ") %> SQL diff --git a/spec/caches_spec.rb b/spec/caches_spec.rb index e9a80d0..f443c24 100644 --- a/spec/caches_spec.rb +++ b/spec/caches_spec.rb @@ -5,9 +5,9 @@ end it 'adds a cache with the given attributes to the @caches ivar' do - query = build(:cached_query, always: true, count: nil, name: nil) + query = build(:cached_query, always: true, count: nil, id: nil, name: nil) - Readyset::Caches.cache(always: true, id: query.id) { query.text } + Readyset::Caches.cache(always: true) { query.text } caches = Readyset::Caches.instance_variable_get(:@caches) expect(caches.size).to eq(1) @@ -16,9 +16,9 @@ context 'when no always parameter is passed' do it 'defaults the always parameter to false' do - query = build(:cached_query, count: nil, name: nil) + query = build(:cached_query, count: nil, id: nil, name: nil) - Readyset::Caches.cache(id: query.id) { query.text } + Readyset::Caches.cache { query.text } always = Readyset::Caches.instance_variable_get(:@caches).first.always expect(always).to eq(false) @@ -28,8 +28,8 @@ describe '.caches' do it 'returns the caches stored in the @caches ivar' do - query = build(:cached_query, count: nil, name: nil) - Readyset::Caches.cache(always: query.always, id: query.id) { query.text } + query = build(:cached_query, count: nil, id: nil, name: nil) + Readyset::Caches.cache(always: query.always) { query.text } result = Readyset::Caches.caches diff --git a/spec/rake_spec.rb b/spec/rake_spec.rb index 4f0abaf..e080994 100644 --- a/spec/rake_spec.rb +++ b/spec/rake_spec.rb @@ -14,8 +14,8 @@ describe 'dump' do it 'dumps the current set of caches to a migration file' do # Setup - allow(Readyset::Query::CachedQuery).to receive(:all). - and_return([build(:cached_query), build(:cached_query_2)]) + cache1 = build_and_create_cache(:cached_query, count: nil, id: nil, name: nil) + cache2 = build_and_create_cache(:cached_query_2, count: nil, id: nil, name: nil) # Execute Rake::Task['readyset:caches:dump'].execute @@ -27,8 +27,8 @@ caches = subclasses.first.caches expect(caches.size).to eq(2) - expect(caches).to include(build(:cached_query, count: nil, name: nil)) - expect(caches).to include(build(:cached_query_2, count: nil, name: nil)) + expect(caches).to include(cache1) + expect(caches).to include(cache2) end end @@ -42,27 +42,33 @@ context "when the migration file contains caches that don't exist on ReadySet" do it "creates the caches in the migration file that don't exist on ReadySet" do # Setup - cache_to_create = build(:cached_query_2) - generate_migration_file([build(:cached_query), cache_to_create]) + existing_cache = build_and_create_cache(:cached_query) + cache_to_create = build_and_create_cache(:cached_query_2) + + Rake::Task['readyset:caches:dump'].execute + cache_to_create.drop! - allow(Readyset::Query::CachedQuery).to receive(:all).and_return([build(:cached_query)]) - allow(Readyset).to receive(:create_cache!).with(id: cache_to_create.id) allow(STDIN).to receive(:gets).and_return("y\n") # Execute Rake::Task['readyset:caches:migrate'].execute # Verify - expect(Readyset).to have_received(:create_cache!).with(id: cache_to_create.id) + caches = Readyset::Query::CachedQuery.all + expect(caches.size).to eq(2) + cache_texts = caches.map(&:text) + expect(cache_texts).to include(existing_cache.text) + expect(cache_texts).to include(cache_to_create.text) end it 'prints the expected output' do # Setup - cache_to_create = build(:cached_query_2) - generate_migration_file([build(:cached_query), cache_to_create]) + build_and_create_cache(:cached_query) + cache_to_create = build_and_create_cache(:cached_query_2) + + Rake::Task['readyset:caches:dump'].execute + cache_to_create.drop! - allow(Readyset::Query::CachedQuery).to receive(:all).and_return([build(:cached_query)]) - allow(Readyset).to receive(:create_cache!).with(id: cache_to_create.id) allow(STDIN).to receive(:gets).and_return("y\n") # Execute + Verify @@ -75,30 +81,27 @@ context "when ReadySet has caches that don't exist in the migration file" do it 'drops the caches that exist on ReadySet that are not in the migration file' do - # Setup - generate_migration_file([build(:cached_query)]) + existing_cache = build_and_create_cache(:cached_query) + Rake::Task['readyset:caches:dump'].execute + build_and_create_cache(:cached_query_2) - cache_to_drop = build(:cached_query_2) - allow(Readyset::Query::CachedQuery).to receive(:all). - and_return([build(:cached_query), cache_to_drop]) - allow(Readyset).to receive(:drop_cache!).with(name_or_id: cache_to_drop.id) allow(STDIN).to receive(:gets).and_return("y\n") # Execute Rake::Task['readyset:caches:migrate'].execute # Verify - expect(Readyset).to have_received(:drop_cache!).with(name_or_id: cache_to_drop.id) + caches = Readyset::Query::CachedQuery.all + cache_texts = caches.map(&:text) + expect(cache_texts).to eq([existing_cache.text]) end it 'prints the expected output' do # Setup - generate_migration_file([build(:cached_query)]) + build_and_create_cache(:cached_query) + Rake::Task['readyset:caches:dump'].execute + build_and_create_cache(:cached_query_2) - cache_to_drop = build(:cached_query_2) - allow(Readyset::Query::CachedQuery).to receive(:all). - and_return([build(:cached_query), cache_to_drop]) - allow(Readyset).to receive(:drop_cache!).with(name_or_id: cache_to_drop.id) allow(STDIN).to receive(:gets).and_return("y\n") # Execute + Verify @@ -108,12 +111,6 @@ to_stdout end end - - def generate_migration_file(caches) - allow(Readyset::Query::CachedQuery).to receive(:all).and_return(caches) - Rake::Task['readyset:caches:dump'].execute - allow(Readyset::Query::CachedQuery).to receive(:all).and_call_original - end end end end