Merge pull request #3244 from mwiencek/ro-connector

MBS-13337: Use standby Postgres instances for read-only queries

admin/InitDb.pl

@@ -410,7 +410,7 @@ sub SanityCheck
     if ($REPTYPE == RT_MIRROR)
     {
         warn "Warning: this is a mirror replication server, but there is no READONLY connection defined\n"
-            unless Databases->get('READONLY');
+            unless Databases->exists('READONLY');
     }
 
     if ($REPTYPE == RT_MASTER)

admin/SetLanguageFrequencies

@@ -23,6 +23,7 @@ use constant PRESELECTED_LANGUAGES => qw(
 
 my $c = MusicBrainz::Server::Context->create_script_context;
 my $sql = $c->sql;
+my $ro_sql = $c->prefer_ro_sql;
 
 #
 # Set the frequency attribute of the language table.
@@ -40,7 +41,7 @@ foreach my $code ( PRESELECTED_LANGUAGES )
 # Calculate language frequencies
 #
 
-$sql->select(<<'EOF', MAX_TOP_LANGUAGES + scalar(PRESELECTED_LANGUAGES));
+$ro_sql->select(<<'EOF', MAX_TOP_LANGUAGES + scalar(PRESELECTED_LANGUAGES));
     SELECT              language, COUNT(*)
     FROM                release
     WHERE NOT   language IS NULL
@@ -51,14 +52,14 @@ EOF
 
 
 # put at most MAX_TOP_LANGUAGES into %languages
-while ( my ($language, $count) = $sql->next_row() )
+while ( my ($language, $count) = $ro_sql->next_row() )
 {
     last if keys(%languages) >= MAX_TOP_LANGUAGES;
 
     $languages{ $language } = 1;
 }
 
-$sql->finish();
+$ro_sql->finish();
 
 print 'New top languages: ', join(' ', keys %languages), "\n";
 

admin/cleanup/FixTrackLength.pl

@@ -51,7 +51,7 @@
 
 # Find mediums with at least one track to fix
 log_info { 'Finding candidate mediums' } if $verbose;
-my @medium_ids = @{ $c->sql->select_single_column_array(
+my @medium_ids = @{ $c->prefer_ro_sql->select_single_column_array(
     'SELECT DISTINCT m.id
        FROM medium m
        JOIN medium_cdtoc mcd ON mcd.medium = m.id

lib/DBDefs.pm.sample

@@ -60,6 +60,7 @@ MusicBrainz::Server::DatabaseConnectionFactory->register_databases(
         password        => 'musicbrainz',
 #       host            => '',
 #       port            => '',
+        read_only   => 1,
     },
     # How to connect for administrative access
     SYSTEM    => {
@@ -86,6 +87,7 @@ MusicBrainz::Server::DatabaseConnectionFactory->register_databases(
 #       password        => '',
 #       host            => '',
 #       port            => '5432',
+#       read_only    => 1,
 #   },
 );
 

lib/DBDefs/Default.pm

@@ -450,6 +450,32 @@ sub DISCOURSE_SSO_SECRET { '' }
 # of security on the MusicBrainz production site.
 sub NONCE_SECRET { '' }
 
+# `USE_RO_DATABASE_CONNECTOR` signals to MusicBrainz Server that it may open
+# an additional database connector per request that it can send read-only
+# queries to. In production this may be used to distribute read-only queries
+# to a PostgreSQL standby instance. (We avoid using this connector on the
+# website if a logged-in user exists, so as not to subject them to potential
+# replication lag while editing.)
+#
+# `USE_RO_DATABASE_CONNECTOR` is not enabled by default, because it's only
+# useful if a separate standby exists. It's also wasteful to potentially open
+# another connector per request if it's otherwise identical to the default
+# connection handle.
+#
+# If enabled, the connector will point to the `READONLY` database.
+#
+# This setting is ignored if `DB_READ_ONLY` is enabled, or if the
+# `REPLICATION_TYPE` is `RT_MIRROR`, because the default connection handle
+# already points to the `READONLY` database in those cases. If it's needed to
+# distribute read-only queries for a mirror database to a standby, haproxy
+# can be used.
+#
+# We don't define which queries the server will send to the RO connector; any
+# transactions that don't perform writes may be redirected, but it depends on
+# which parts of the server code have been updated to actually use the
+# connector.
+sub USE_RO_DATABASE_CONNECTOR { 0 }
+
 # When enabled, if Catalyst receives a request with an `mb-set-database`
 # header, all database queries will go to the specified database instead of
 # READWRITE, as defined in the DatabaseConnectionFactory->register_databases

lib/MusicBrainz/Script/RemoveEmpty.pm

@@ -101,7 +101,7 @@ sub run {
     my ($count, $removed) = (0, 0);
     my @entities = values %{
         $self->c->model(type_to_model($entity))->get_by_ids(
-            @{ $self->c->sql->select_single_column_array($query) },
+            @{ $self->c->prefer_ro_sql->select_single_column_array($query) },
         );
     };
     my $modbot = $self->c->model('Editor')->get_by_id($EDITOR_MODBOT);

lib/MusicBrainz/Script/RemoveUnreferencedRows.pm

@@ -48,7 +48,7 @@ sub run {
     log_info { 'Checking all unreferenced rows.' };
 
     my @unreferenced_rows = @{
-        $self->c->sql->select_list_of_lists(<<~'SQL');
+        $self->c->prefer_ro_sql->select_list_of_lists(<<~'SQL');
               SELECT table_name,
                      array_agg(row_id) AS row_ids
                 FROM unreferenced_row_log

lib/MusicBrainz/Server.pm

@@ -423,6 +423,9 @@ before dispatch => sub {
 
     my $ctx = $self->model('MB')->context;
 
+    # Provide access to the Catalyst context in the data layer.
+    $ctx->catalyst_context($self);
+
     # The mb-set-database header is added by:
     #  1) the http proxy in t/selenium.mjs
     #  2) make_jsonld_request in
@@ -435,6 +438,8 @@ before dispatch => sub {
         no warnings 'redefine';
         $ctx->database($database);
         $ctx->clear_connector;
+        $ctx->ro_database('READONLY_' . $database);
+        $ctx->clear_ro_connector;
         my $cache_namespace = DBDefs->CACHE_NAMESPACE;
         *DBDefs::CACHE_NAMESPACE = sub { $cache_namespace . $database . ':' };
         *DBDefs::ENTITY_CACHE_TTL = sub { 1 };
@@ -448,7 +453,9 @@ before dispatch => sub {
     } else {
         # Use a fresh database connection for every request, and
         # remember to disconnect at the end.
-        $ctx->connector->refresh;
+        $ctx->connector->refresh if $ctx->has_connector;
+        $ctx->ro_connector->refresh
+            if $ctx->has_ro_connector && defined $ctx->ro_connector;
     }
 
     # Any time `TO_JSON` is called on an Entity, it may add other
@@ -463,7 +470,10 @@ after dispatch => sub {
 
     my $ctx = $self->model('MB')->context;
 
-    $ctx->connector->disconnect;
+    $ctx->catalyst_context(undef);
+    $ctx->connector->disconnect if $ctx->has_connector;
+    $ctx->ro_connector->disconnect
+        if $ctx->has_ro_connector && defined $ctx->ro_connector;
     $ctx->store->disconnect;
     $ctx->cache->disconnect;
 
@@ -474,6 +484,8 @@ after dispatch => sub {
         # back to the default (READWRITE, or READONLY for mirrors).
         $ctx->clear_connector;
         $ctx->clear_database;
+        $ctx->clear_ro_connector;
+        $ctx->clear_ro_database;
         $ctx->clear_cache_manager;
         $ctx->clear_store;
         *DBDefs::CACHE_NAMESPACE = $ORIG_CACHE_NAMESPACE;

lib/MusicBrainz/Server/Connector.pm

@@ -11,6 +11,7 @@ has 'conn' => (
     handles    => [qw( dbh )],
     lazy_build => 1,
     clearer => '_clear_conn',
+    predicate => 'has_conn',
 );
 
 has 'database' => (
@@ -22,12 +23,20 @@ has 'sql' => (
     is => 'ro',
     default => sub {
         my $self = shift;
-        Sql->new( $self->conn );
+        my $sql = Sql->new( $self->conn );
+        $sql->read_only($self->read_only);
+        return $sql;
     },
     lazy => 1,
     clearer => '_clear_sql',
 );
 
+has 'read_only' => (
+    is => 'ro',
+    isa => 'Bool',
+    default => 0,
+);
+
 sub _build_conn
 {
     my ($self) = @_;
@@ -62,8 +71,8 @@ sub _build_conn
 
 sub _disconnect {
     my ($self) = @_;
-    if (my $conn = $self->conn) {
-        $conn->disconnect;
+    if ($self->has_conn) {
+        $self->conn->disconnect;
     }
 
     $self->_clear_conn;

lib/MusicBrainz/Server/Context.pm

@@ -29,20 +29,73 @@ has 'connector' => (
     handles => [ 'dbh', 'sql', 'conn' ],
     lazy_build => 1,
     clearer => 'clear_connector',
+    predicate => 'has_connector',
 );
 
+has 'ro_connector' => (
+    is => 'ro',
+    lazy_build => 1,
+    clearer => 'clear_ro_connector',
+    predicate => 'has_ro_connector',
+);
+
+sub is_globally_read_only {
+    return DBDefs->DB_READ_ONLY || DBDefs->REPLICATION_TYPE == RT_MIRROR;
+}
+
+sub prefer_ro_connector {
+    my ($self) = @_;
+    # There are two cases where we cannot, or do not want to use the
+    # `ro_connector` (if available):
+    #  * Case 1:
+    #    There is a logged-in user. We want to ensure replication lag (no
+    #    matter how minimal) doesn't prevent just-applied edits from being
+    #    visible to them.
+    #  * Case 2:
+    #    We're in a current writable transaction. We should of course perform
+    #    our query in the same transaction for consistency and atomicity.
+    if (
+        (
+            defined $self->catalyst_context &&
+            $self->catalyst_context->user_exists
+        ) ||
+        (
+            $self->has_connector &&
+            $self->connector->sql->is_in_transaction
+        )
+    ) {
+        return $self->connector;
+    }
+    # `ro_connector` may be undef; see `_build_ro_connector`.
+    return $self->ro_connector // $self->connector;
+}
+
+sub prefer_ro_dbh { shift->prefer_ro_connector->dbh }
+
+sub prefer_ro_sql { shift->prefer_ro_connector->sql }
+
+sub prefer_ro_conn { shift->prefer_ro_connector->conn }
+
 has 'database' => (
     is => 'rw',
     isa => 'Str',
     default => sub {
-        DBDefs->DB_READ_ONLY || DBDefs->REPLICATION_TYPE == RT_MIRROR
+        shift->is_globally_read_only
             ? 'READONLY'
             : 'READWRITE';
     },
     lazy => 1,
     clearer => 'clear_database',
 );
 
+has 'ro_database' => (
+    is => 'rw',
+    isa => 'Str',
+    default => sub { 'READONLY' },
+    lazy => 1,
+    clearer => 'clear_ro_database',
+);
+
 has 'fresh_connector' => (
     is => 'ro',
     isa => 'Bool',
@@ -65,6 +118,21 @@ sub _build_connector {
     return $conn;
 }
 
+sub _build_ro_connector {
+    my $self = shift;
+
+    return unless (
+        DBDefs->USE_RO_DATABASE_CONNECTOR &&
+        !$self->is_globally_read_only
+    );
+
+    return DatabaseConnectionFactory->get_connection(
+        $self->ro_database,
+        fresh => $self->fresh_connector,
+        read_only => 1,
+    );
+}
+
 has 'models' => (
     isa     => 'HashRef',
     is      => 'ro',
@@ -140,8 +208,38 @@ has 'max_request_time' => (
     isa => 'Maybe[Int]',
 );
 
+has 'catalyst_context' => (
+    is => 'rw',
+    isa => 'Maybe[MusicBrainz::Server]',
+    weak_ref => 1,
+);
+
 1;
 
+=head1 ATTRIBUTES
+
+=head2 ro_database
+
+String referencing a database name in C<DBDefs> that may be used to
+distribute read-only queries to PostgreSQL standby instance if
+`USE_RO_DATABASE_CONNECTOR` is enabled.
+
+=head2 ro_connector
+
+Connector (to `ro_database`) that may be used for read-only transactions.
+
+=head2 catalyst_context
+
+Provides access to the current Catalyst context. This should be used
+sparingly to avoid coupling the controller and data layers.
+
+=head1 METHODS
+
+=head2 prefer_ro_connector()
+
+Returns `ro_connector` unless `connector` is in a transaction, or unless
+there's a logged-in user.
+
 =head1 COPYRIGHT AND LICENSE
 
 Copyright (C) 2009 Lukas Lalinsky

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

@@ -14,8 +14,6 @@ role {
     my $params = shift;
     my $table = $params->table;
 
-    requires '_dbh';
-
     method 'adjust_edit_pending' => sub
     {
         my ($self, $adjust, @ids) = @_;

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

@@ -16,7 +16,7 @@ sub query_to_list {
 
     map {
         $builder->($self, $_)
-    } @{ $self->c->sql->select_list_of_hashes($query, @$args) };
+    } @{ $self->c->prefer_ro_sql->select_list_of_hashes($query, @$args) };
 }
 
 sub query_to_list_limited {
@@ -77,7 +77,7 @@ sub query_to_list_limited {
         my $row = $_;
         $total_row_count = delete $row->{total_row_count};
         $builder->($self, $row);
-    } @{$self->c->sql->select_list_of_hashes($query, @args)};
+    } @{$self->c->prefer_ro_sql->select_list_of_hashes($query, @args)};
 
     if (!defined $hits) {
         $hits = ($total_row_count // 0) + ($offset // 0);

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

@@ -9,7 +9,7 @@ parameter 'order_by' => (
 
 role
 {
-    requires '_columns', '_table', '_dbh', '_new_from_row', '_type';
+    requires '_columns', '_table', '_new_from_row', '_type';
 
     my $params = shift;
 

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

@@ -9,9 +9,8 @@ sub sql {
     return $self->c->sql;
 }
 
-sub _dbh
-{
-    shift->c->dbh;
-}
+sub ro_sql { shift->c->ro_connector->sql }
+
+sub prefer_ro_sql { shift->c->prefer_ro_sql }
 
 1;