Merge pull request #3172 from mwiencek/mbs-13449-no-cache-add-delay

Don't delay cache additions until after the transaction ends

lib/MusicBrainz/Server/Data/ArtistCredit.pm

@@ -16,11 +16,14 @@ with 'MusicBrainz::Server::Data::Role::EntityCache',
      'MusicBrainz::Server::Data::Role::PendingEdits' => {
         table => 'artist_credit',
      },
-     'MusicBrainz::Server::Data::Role::GetByGID';
+     'MusicBrainz::Server::Data::Role::GetByGID',
+     'MusicBrainz::Server::Data::Role::GIDRedirect';
+
+sub _main_table { 'artist_credit' }
 
 sub _type { 'artist_credit' }
 
-sub _table { shift->_type }
+sub _table { shift->_main_table }
 
 sub _gid_redirect_table { shift->_table . '_gid_redirect' }
 
@@ -396,76 +399,6 @@ sub related_entities {
     return $related;
 }
 
-sub load_gid_redirects {
-    my ($self, @entities) = @_;
-    my $table = $self->_gid_redirect_table;
-
-    my %entities_by_id = object_to_ids(@entities);
-
-    my $query = "SELECT new_id, array_agg(gid) AS gid_redirects FROM $table WHERE new_id = any(?) GROUP BY new_id";
-    my $results = $self->c->sql->select_list_of_hashes($query, [map { $_->id } @entities]);
-
-    for my $row (@$results) {
-        for my $entity (@{ $entities_by_id{$row->{new_id}} }) {
-            $entity->gid_redirects($row->{gid_redirects});
-        }
-    }
-}
-
-sub remove_gid_redirects
-{
-    my ($self, @ids) = @_;
-    my $table = $self->_gid_redirect_table;
-    $self->sql->do("DELETE FROM $table WHERE new_id IN (" . placeholders(@ids) . ')', @ids);
-}
-
-sub add_gid_redirects
-{
-    my ($self, %redirects) = @_;
-    my $table = $self->_gid_redirect_table;
-    my $query = "INSERT INTO $table (gid, new_id) VALUES " .
-                (join ', ', ('(?, ?)') x keys %redirects);
-    $self->sql->do($query, %redirects);
-}
-
-sub update_gid_redirects
-{
-    my ($self, $new_id, @old_ids) = @_;
-    my $table = $self->_gid_redirect_table;
-    $self->sql->do("
-        UPDATE $table SET new_id = ?
-        WHERE new_id IN (".placeholders(@old_ids).')', $new_id, @old_ids);
-}
-
-sub _delete_and_redirect_gids
-{
-    my ($self, $table, $new_id, @old_ids) = @_;
-
-    # Update all GID redirects from @old_ids to $new_id
-    $self->update_gid_redirects($new_id, @old_ids);
-
-    # Delete the recording and select current GIDs
-    my $old_gids = $self->delete_returning_gids(@old_ids);
-
-    # Add redirects from GIDs of the deleted recordings to $new_id
-    $self->add_gid_redirects(map { $_ => $new_id } @$old_gids);
-
-    if ($self->can('_delete_from_cache')) {
-        $self->_delete_from_cache(
-            $new_id, @old_ids,
-            @$old_gids,
-        );
-    }
-}
-
-sub delete_returning_gids {
-    my ($self, @ids) = @_;
-    return $self->sql->select_single_column_array('
-        DELETE FROM ' . $self->_table . '
-        WHERE id IN ('.placeholders(@ids).')
-        RETURNING gid', @ids);
-}
-
 __PACKAGE__->meta->make_immutable;
 1;
 

lib/MusicBrainz/Server/Data/Event.pm

@@ -78,12 +78,18 @@ sub update
 
     $self->sql->update_row('event', $row, { id => $event_id }) if %$row;
 
+    return 1;
+}
+
+# This is run `after` so that cache invalidation happens first
+# (via `Data::Role::EntityCache``).
+after update => sub {
+    my ($self, $event_id, $update) = @_;
+
     if (any { exists $update->{$_} } qw( name begin_date end_date time )) {
         $self->c->model('Series')->reorder_for_entities('event', $event_id);
     }
-
-    return 1;
-}
+};
 
 sub can_delete { 1 }
 

lib/MusicBrainz/Server/Data/Release.pm

@@ -893,15 +893,21 @@ sub update {
             SQL
     }
 
-    if ($update->{events} || $update->{name}) {
-        $self->c->model('Series')->reorder_for_entities('release', $release_id);
-    }
-
     if ($update->{events} || $new_release_group_id) {
         $self->c->model('Series')->reorder_for_entities('release_group', $release_group_id);
     }
 }
 
+# This is run `after` so that cache invalidation happens first
+# (via `Data::Role::EntityCache``).
+after update => sub {
+    my ($self, $release_id, $update) = @_;
+
+    if ($update->{events} || $update->{name}) {
+        $self->c->model('Series')->reorder_for_entities('release', $release_id);
+    }
+};
+
 sub can_delete { 1 }
 
 sub delete

lib/MusicBrainz/Server/Data/ReleaseGroup.pm

@@ -676,11 +676,17 @@ sub update {
     $self->sql->update_row('release_group', $row, { id => $group_id }) if %$row;
     $self->c->model('ReleaseGroupSecondaryType')->set_types($group_id, $update->{secondary_type_ids})
         if exists $update->{secondary_type_ids};
+}
+
+# This is run `after` so that cache invalidation happens first
+# (via `Data::Role::EntityCache``).
+after update => sub {
+    my ($self, $group_id, $update) = @_;
 
     if ($update->{name}) {
         $self->c->model('Series')->reorder_for_entities('release_group', $group_id);
     }
-}
+};
 
 sub can_delete
 {

lib/MusicBrainz/Server/Data/ReleaseLabel.pm

@@ -136,7 +136,6 @@ sub insert
 
     my $release_id = $row->{release};
     $self->c->model('Series')->reorder_for_entities('release', $release_id);
-    $self->c->model('Release')->_delete_from_cache($release_id);
 
     return wantarray ? @created : $created[0];
 }
@@ -155,7 +154,6 @@ sub update
     );
 
     $self->c->model('Series')->reorder_for_entities('release', $release_id);
-    $self->c->model('Release')->_delete_from_cache($release_id);
 }
 
 sub delete {
@@ -167,7 +165,6 @@ sub delete {
     );
 
     $self->c->model('Series')->reorder_for_entities('release', @$release_ids);
-    $self->c->model('Release')->_delete_from_cache(@$release_ids);
 }
 
 __PACKAGE__->meta->make_immutable;

lib/MusicBrainz/Server/Data/Role/EntityCache.pm

@@ -20,18 +20,14 @@ the following sequence of events:
 
  1. Request A updates artist ID=123 and calls _delete_from_cache(123). In our
     persistent Redis store, we set `artist:recently_invalidated:123` to '1'.
-    In our Redis cache, we delete `artist:123`. (While cache addition is
-    delayed until after the relevant transaction commits, cache deletion is
-    performed immediately.)
+    In our Redis cache, we delete `artist:123`.
 
  2. Request B starts before request A's database transaction commits, and
     attempts to load artist ID=123. Since it's not in the cache, we read it
-    from the database, seeing the "old" version.
+    from the database, seeing the "old" version, and add the stale artist to
+    the cache at `artist:123`.
 
- 3. Request B commits, and adds the stale artist to the cache at
-    `artist:123`.
-
- 4. Immediately after adding it to the cache, we check for the presence of an
+ 3. Immediately after adding it to the cache, we check for the presence of an
     `artist:recently_invalidated:123` key in the Redis store. If it exists,
     we remove the stale `artist:123` entry we just added to the cache.
 
@@ -125,20 +121,7 @@ sub _add_to_cache {
     my @entries = $self->_create_cache_entries($data, $ids);
     return unless @entries;
 
-    my $sql = $self->c->sql;
-    if ($sql->is_in_transaction) {
-        $sql->add_post_txn_callback(sub {
-            $self->_add_to_cache_impl($cache, \@entries, $ids);
-        });
-    } else {
-        $self->_add_to_cache_impl($cache, \@entries, $ids);
-    }
-}
-
-sub _add_to_cache_impl {
-    my ($self, $cache, $entries, $ids) = @_;
-
-    $cache->set_multi(@$entries);
+    $cache->set_multi(@entries);
 
     # Check if any entities we've just cached were recently invalidated.
     # If so, delete them. This must be performed after the cache

lib/MusicBrainz/Server/Data/Role/GIDRedirect.pm

@@ -126,16 +126,24 @@ sub remove_gid_redirects
 {
     my ($self, @ids) = @_;
     my $table = $self->_gid_redirect_table;
-    $self->sql->do("DELETE FROM $table WHERE new_id IN (" . placeholders(@ids) . ')', @ids);
+    my $gids = $self->sql->select_single_column_array(<<~"SQL", \@ids);
+        DELETE FROM $table WHERE new_id = any(?) RETURNING gid
+        SQL
+    if ($self->can('_delete_from_cache')) {
+        $self->_delete_from_cache(@$gids);
+    }
 }
 
 sub update_gid_redirects
 {
     my ($self, $new_id, @old_ids) = @_;
     my $table = $self->_gid_redirect_table;
-    $self->sql->do("
-        UPDATE $table SET new_id = ?
-        WHERE new_id IN (" . placeholders(@old_ids) . ')', $new_id, @old_ids);
+    my $gids = $self->sql->select_single_column_array(<<~"SQL", $new_id, \@old_ids);
+        UPDATE $table SET new_id = ? WHERE new_id = any(?) RETURNING gid
+        SQL
+    if ($self->can('_delete_from_cache')) {
+        $self->_delete_from_cache(@$gids);
+    }
 }
 
 

lib/Sql.pm

@@ -47,28 +47,6 @@ has 'sth' => (
     clearer => 'clear_sth',
 );
 
-# Note: These callbacks run regardless of whether the transaction succeeded!
-has 'post_txn_callbacks' => (
-    traits  => ['Array'],
-    is => 'rw',
-    isa => 'ArrayRef[CodeRef]',
-    default => sub { [] },
-);
-
-sub add_post_txn_callback {
-    my ($self, $callback) = @_;
-    croak 'add_post_txn_callback called without begin'
-        unless $self->is_in_transaction;
-    my $index = $self->transaction_depth - 1;
-    push @{ $self->post_txn_callbacks->[$index] }, $callback;
-}
-
-sub _run_post_txn_callbacks {
-    my ($self) = @_;
-    my $callbacks = pop @{ $self->post_txn_callbacks };
-    $_->() for @$callbacks;
-}
-
 sub finish
 {
     my ($self) = @_;
@@ -253,8 +231,8 @@ sub delete_row
 
 # We don't support nested transactions (i.e., we don't make use of
 # savepoints), but we do fake them in order to allow tests that use both
-# `t::Context` and `t::Mechanize` to actually function. Besides running the
-# `post_txn_callbacks`, nested commits and rollbacks are simply ignored.
+# `t::Context` and `t::Mechanize` to actually function. Nested commits and
+# rollbacks are simply ignored.
 has 'transaction_depth' => (
     isa => 'Int',
     is => 'ro',
@@ -270,7 +248,6 @@ sub begin
 {
     my $self = shift;
     $self->dbh->{AutoCommit} = 0;
-    push @{ $self->post_txn_callbacks }, [];
     my $txn_depth = $self->inc_transaction_depth;
     if ($txn_depth == 1) {
         return Sql::Timer->new('BEGIN', []) if $self->debug;
@@ -284,7 +261,6 @@ sub commit
 
     my $txn_depth = $self->dec_transaction_depth;
     if ($txn_depth > 0) {
-        $self->_run_post_txn_callbacks;
         return;
     }
 
@@ -304,17 +280,6 @@ sub commit
         cluck $err unless ($self->quiet);
         eval { $self->rollback };
         croak $err;
-    }
-    finally {
-        # Note: exceptions in `finally` blocks cannot be propagated "due to
-        # fundamental limitations of Perl."
-        #
-        # Never use `post_txn_callbacks` for anything that MUST complete
-        # to preserve database or cache consistency.
-        #
-        # Aside: we could pass any caught error here if any callback needs to
-        # determine whether the commit succeeded in the future.
-        $self->_run_post_txn_callbacks;
     };
 }
 
@@ -325,7 +290,6 @@ sub rollback
 
     my $txn_depth = $self->dec_transaction_depth;
     if ($txn_depth > 0) {
-        $self->_run_post_txn_callbacks;
         return;
     }
 
@@ -344,10 +308,6 @@ sub rollback
         $self->dbh->{AutoCommit} = 1;
         cluck $err unless $self->quiet;
         croak $err;
-    }
-    finally {
-        # See the comment in `commit` above.
-        $self->_run_post_txn_callbacks;
     };
 }
 

t/lib/t/MusicBrainz/Server/Data/Release.pm

@@ -823,4 +823,20 @@ test 'Merging releases with the same date should discard unknown country events'
        'no unknown release events');
 };
 
+test 'Release events are not cached on the release' => sub {
+    my $test = shift;
+    my $c = $test->c;
+
+    MusicBrainz::Server::Test->prepare_test_database($c, '+releaselabel');
+
+    # Ensure the release is cached.
+    $c->sql->begin;
+    my $release = $c->model('Release')->get_by_id(1);
+    $c->model('Release')->load_release_events($release);
+    $c->sql->commit;
+
+    $release = $c->cache->get('release:1'),
+    is(scalar($release->all_events), 0, 'cached release has no events');
+};
+
 1;