From 3ef245b21a397e5253ba1bda607a1510245d5c09 Mon Sep 17 00:00:00 2001 From: Byron Jones Date: Mon, 7 Apr 2014 16:38:31 +0800 Subject: Bug 987032: allow memcached to cache bugzilla configuration information r=dkl, a=glob --- Bugzilla/Classification.pm | 3 ++ Bugzilla/Field.pm | 3 ++ Bugzilla/Field/Choice.pm | 2 + Bugzilla/Group.pm | 2 + Bugzilla/Keyword.pm | 2 + Bugzilla/Memcached.pm | 124 +++++++++++++++++++++++++++++++++++++++----- Bugzilla/Milestone.pm | 1 + Bugzilla/Object.pm | 80 ++++++++++++++++++++-------- Bugzilla/Product.pm | 2 + Bugzilla/Status.pm | 20 +++++-- Bugzilla/User.pm | 20 +++++-- contrib/convert-workflow.pl | 1 + editclassifications.cgi | 1 + 13 files changed, 216 insertions(+), 45 deletions(-) diff --git a/Bugzilla/Classification.pm b/Bugzilla/Classification.pm index 6e88bdc63..0ae548bb6 100644 --- a/Bugzilla/Classification.pm +++ b/Bugzilla/Classification.pm @@ -23,6 +23,8 @@ use parent qw(Bugzilla::Field::ChoiceInterface Bugzilla::Object Exporter); #### Initialization #### ############################### +use constant IS_CONFIG => 1; + use constant DB_TABLE => 'classifications'; use constant LIST_ORDER => 'sortkey, name'; @@ -67,6 +69,7 @@ sub remove_from_db { foreach my $id (@$product_ids) { Bugzilla->memcached->clear({ table => 'products', id => $id }); } + Bugzilla->memcached->clear_config(); } $self->SUPER::remove_from_db(); diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm index 1c9927bf4..6289f110a 100644 --- a/Bugzilla/Field.pm +++ b/Bugzilla/Field.pm @@ -74,6 +74,8 @@ use Scalar::Util qw(blessed); #### Initialization #### ############################### +use constant IS_CONFIG => 1; + use constant DB_TABLE => 'fielddefs'; use constant LIST_ORDER => 'sortkey, name'; @@ -1078,6 +1080,7 @@ sub create { unless $is_obsolete; Bugzilla->memcached->clear({ table => 'fielddefs', id => $field->id }); + Bugzilla->memcached->clear_config(); } return $field; diff --git a/Bugzilla/Field/Choice.pm b/Bugzilla/Field/Choice.pm index b3bca0dd2..6f730072f 100644 --- a/Bugzilla/Field/Choice.pm +++ b/Bugzilla/Field/Choice.pm @@ -24,6 +24,8 @@ use Scalar::Util qw(blessed); # Initialization # ################## +use constant IS_CONFIG => 1; + use constant DB_COLUMNS => qw( id value diff --git a/Bugzilla/Group.pm b/Bugzilla/Group.pm index 25a792417..534313d9c 100644 --- a/Bugzilla/Group.pm +++ b/Bugzilla/Group.pm @@ -21,6 +21,8 @@ use Bugzilla::Config qw(:admin); ##### Module Initialization ### ############################### +use constant IS_CONFIG => 1; + use constant DB_COLUMNS => qw( groups.id groups.name diff --git a/Bugzilla/Keyword.pm b/Bugzilla/Keyword.pm index 77c9f5f77..a8c098854 100644 --- a/Bugzilla/Keyword.pm +++ b/Bugzilla/Keyword.pm @@ -19,6 +19,8 @@ use Bugzilla::Util; #### Initialization #### ############################### +use constant IS_CONFIG => 1; + use constant DB_COLUMNS => qw( keyworddefs.id keyworddefs.name diff --git a/Bugzilla/Memcached.pm b/Bugzilla/Memcached.pm index 0752bcce9..2bf7f1393 100644 --- a/Bugzilla/Memcached.pm +++ b/Bugzilla/Memcached.pm @@ -40,6 +40,10 @@ sub _new { return bless($self, $class); } +sub enabled { + return $_[0]->{memcached} ? 1 : 0; +} + sub set { my ($self, $args) = @_; return unless $self->{memcached}; @@ -95,6 +99,32 @@ sub get { } } +sub set_config { + my ($self, $args) = @_; + return unless $self->{memcached}; + + if (exists $args->{key}) { + return $self->_set($self->_config_prefix . ':' . $args->{key}, $args->{data}); + } + else { + ThrowCodeError('params_required', { function => "Bugzilla::Memcached::set_config", + params => [ 'key' ] }); + } +} + +sub get_config { + my ($self, $args) = @_; + return unless $self->{memcached}; + + if (exists $args->{key}) { + return $self->_get($self->_config_prefix . ':' . $args->{key}); + } + else { + ThrowCodeError('params_required', { function => "Bugzilla::Memcached::get_config", + params => [ 'key' ] }); + } +} + sub clear { my ($self, $args) = @_; return unless $self->{memcached}; @@ -130,39 +160,61 @@ sub clear { sub clear_all { my ($self) = @_; - return unless my $memcached = $self->{memcached}; - if (!$memcached->incr("prefix", 1)) { - $memcached->add("prefix", time()); - } + return unless $self->{memcached}; + $self->_inc_prefix("global"); +} + +sub clear_config { + my ($self) = @_; + return unless $self->{memcached}; + $self->_inc_prefix("config"); } # in order to clear all our keys, we add a prefix to all our keys. when we # need to "clear" all current keys, we increment the prefix. sub _prefix { - my ($self) = @_; + my ($self, $name) = @_; # we don't want to change prefixes in the middle of a request my $request_cache = Bugzilla->request_cache; - if (!$request_cache->{memcached_prefix}) { + my $request_cache_key = "memcached_prefix_$name"; + if (!$request_cache->{$request_cache_key}) { my $memcached = $self->{memcached}; - my $prefix = $memcached->get("prefix"); + my $prefix = $memcached->get($name); if (!$prefix) { $prefix = time(); - if (!$memcached->add("prefix", $prefix)) { + if (!$memcached->add($name, $prefix)) { # if this failed, either another process set the prefix, or # memcached is down. assume we lost the race, and get the new # value. if that fails, memcached is down so use a dummy # prefix for this request. - $prefix = $memcached->get("prefix") || 0; + $prefix = $memcached->get($name) || 0; } } - $request_cache->{memcached_prefix} = $prefix; + $request_cache->{$request_cache_key} = $prefix; } - return $request_cache->{memcached_prefix}; + return $request_cache->{$request_cache_key}; +} + +sub _inc_prefix { + my ($self, $name) = @_; + my $memcached = $self->{memcached}; + if (!$memcached->incr($name, 1)) { + $memcached->add($name, time()); + } + delete Bugzilla->request_cache->{"memcached_prefix_$name"}; +} + +sub _global_prefix { + return $_[0]->_prefix("global"); +} + +sub _config_prefix { + return $_[0]->_prefix("config"); } sub _encode_key { my ($self, $key) = @_; - $key = $self->_prefix . ':' . uri_escape_utf8($key); + $key = $self->_global_prefix . ':' . uri_escape_utf8($key); return length($self->{namespace} . $key) > MAX_KEY_LENGTH ? undef : $key; @@ -175,6 +227,7 @@ sub _set { ThrowCodeError('param_invalid', { function => "Bugzilla::Memcached::set", param => "value" }); } + $key = $self->_encode_key($key) or return; return $self->{memcached}->set($key, $value); @@ -191,10 +244,19 @@ sub _get { # detaint returned values # hashes and arrays are detainted just one level deep if (ref($value) eq 'HASH') { - map { defined($_) && trick_taint($_) } values %$value; + _detaint_hashref($value); } elsif (ref($value) eq 'ARRAY') { - trick_taint($_) foreach @$value; + foreach my $value (@$value) { + next unless defined $value; + # arrays of hashes are common + if (ref($value) eq 'HASH') { + _detaint_hashref($value); + } + elsif (!ref($value)) { + trick_taint($value); + } + } } elsif (!ref($value)) { trick_taint($value); @@ -202,6 +264,15 @@ sub _get { return $value; } +sub _detaint_hashref { + my ($hashref) = @_; + foreach my $value (values %$hashref) { + if (defined($value) && !ref($value)) { + trick_taint($value); + } + } +} + sub _delete { my ($self, $key) = @_; $key = $self->_encode_key($key) @@ -266,6 +337,14 @@ Lmemcached()|Bugzilla/memcached>. =head1 METHODS +=over + +=item C + +Returns true if Memcached support is available and enabled. + +=back + =head2 Setting Adds a value to Memcached. @@ -285,6 +364,13 @@ to C. This is a convenience method which allows cached data to be later retrieved by specifying the C and either the C or C. +=item C $key, data =E $data })> + +Adds the C using the C while identifying the data as part of +Bugzilla's configuration (such as fields, products, components, groups, etc). +Values set with C are automatically cleared when changes are made +to Bugzilla's configuration. + =back =head2 Getting @@ -306,6 +392,11 @@ Return C with the specified C
and C. Return C with the specified C
and C. +=item C $key })> + +Return C with the specified C from the configuration cache. See +C for more information. + =back =head2 Clearing @@ -328,6 +419,11 @@ corresponding C
and C entry. Removes C with the specified C
and C, as well as the corresponding C
and C entry. +=item C + +Removes all configuration related values from the cache. See C for +more information. + =item C Removes all values from the cache. diff --git a/Bugzilla/Milestone.pm b/Bugzilla/Milestone.pm index b1783de88..daa362c34 100644 --- a/Bugzilla/Milestone.pm +++ b/Bugzilla/Milestone.pm @@ -114,6 +114,7 @@ sub update { WHERE id = ? AND defaultmilestone = ?', undef, ($self->name, $self->product_id, $changes->{value}->[0])); Bugzilla->memcached->clear({ table => 'produles', id => $self->product_id }); + Bugzilla->memcached->clear_config(); } $dbh->bz_commit_transaction(); diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm index ff1454539..e5704b3b1 100644 --- a/Bugzilla/Object.pm +++ b/Bugzilla/Object.pm @@ -38,6 +38,11 @@ use constant AUDIT_REMOVES => 1; # Memcached. See documentation in Bugzilla::Memcached for more information. use constant USE_MEMCACHED => 1; +# When IS_CONFIG is true, the class is used to track seldom changed +# configuration objects. This includes, but is not limited to, fields, field +# values, keywords, products, classifications, priorities, severities, etc. +use constant IS_CONFIG => 0; + # This allows the JSON-RPC interface to return Bugzilla::Object instances # as though they were hashes. In the future, this may be modified to return # less information. @@ -56,7 +61,7 @@ sub new { return $object if $object; my ($data, $set_memcached); - if (Bugzilla->feature('memcached') + if (Bugzilla->memcached->enabled && $class->USE_MEMCACHED && ref($param) eq 'HASH' && $param->{cache}) { @@ -352,22 +357,42 @@ sub _do_list_select { my $cols = join(',', $class->_get_db_columns); my $order = $class->LIST_ORDER; - my $sql = "SELECT $cols FROM $table"; - if (defined $where) { - $sql .= " WHERE $where "; + # Unconditional requests for configuration data are cacheable. + my ($objects, $set_memcached, $memcached_key); + if (!defined $where + && Bugzilla->memcached->enabled + && $class->IS_CONFIG) + { + $memcached_key = "$class:get_all"; + $objects = Bugzilla->memcached->get_config({ key => $memcached_key }); + $set_memcached = $objects ? 0 : 1; } - $sql .= " ORDER BY $order"; - - $sql .= " $postamble" if $postamble; - - my $dbh = Bugzilla->dbh; - # Sometimes the values are tainted, but we don't want to untaint them - # for the caller. So we copy the array. It's safe to untaint because - # they're only used in placeholders here. - my @untainted = @{ $values || [] }; - trick_taint($_) foreach @untainted; - my $objects = $dbh->selectall_arrayref($sql, {Slice=>{}}, @untainted); - $class->_serialisation_keys($objects->[0]) if @$objects; + + if (!$objects) { + my $sql = "SELECT $cols FROM $table"; + if (defined $where) { + $sql .= " WHERE $where "; + } + $sql .= " ORDER BY $order"; + $sql .= " $postamble" if $postamble; + + my $dbh = Bugzilla->dbh; + # Sometimes the values are tainted, but we don't want to untaint them + # for the caller. So we copy the array. It's safe to untaint because + # they're only used in placeholders here. + my @untainted = @{ $values || [] }; + trick_taint($_) foreach @untainted; + $objects = $dbh->selectall_arrayref($sql, {Slice=>{}}, @untainted); + $class->_serialisation_keys($objects->[0]) if @$objects; + } + + if ($objects && $set_memcached) { + Bugzilla->memcached->set_config({ + key => $memcached_key, + data => $objects + }); + } + foreach my $object (@$objects) { $object = $class->new_from_hash($object); } @@ -495,8 +520,11 @@ sub update { $self->audit_log(\%changes) if $self->AUDIT_UPDATES; $dbh->bz_commit_transaction(); - Bugzilla->memcached->clear({ table => $table, id => $self->id }) - if $self->USE_MEMCACHED && @values; + if ($self->USE_MEMCACHED && @values) { + Bugzilla->memcached->clear({ table => $table, id => $self->id }); + Bugzilla->memcached->clear_config() + if $self->IS_CONFIG; + } $self->_object_cache_remove({ id => $self->id }); $self->_object_cache_remove({ name => $self->name }) if $self->name; @@ -517,8 +545,11 @@ sub remove_from_db { $self->audit_log(AUDIT_REMOVE) if $self->AUDIT_REMOVES; $dbh->do("DELETE FROM $table WHERE $id_field = ?", undef, $self->id); $dbh->bz_commit_transaction(); - Bugzilla->memcached->clear({ table => $table, id => $self->id }) - if $self->USE_MEMCACHED; + if ($self->USE_MEMCACHED) { + Bugzilla->memcached->clear({ table => $table, id => $self->id }); + Bugzilla->memcached->clear_config() + if $self->IS_CONFIG; + } $self->_object_cache_remove({ id => $self->id }); $self->_object_cache_remove({ name => $self->name }) if $self->name; undef $self; @@ -585,6 +616,13 @@ sub create { my $object = $class->insert_create_data($field_values); $dbh->bz_commit_transaction(); + if (Bugzilla->memcached->enabled + && $class->USE_MEMCACHED + && $class->IS_CONFIG) + { + Bugzilla->memcached->clear_config(); + } + return $object; } @@ -680,7 +718,7 @@ sub insert_create_data { sub get_all { my $class = shift; - return @{$class->_do_list_select()}; + return @{ $class->_do_list_select() }; } ############################### diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm index c3c3416b9..55c4de0b8 100644 --- a/Bugzilla/Product.pm +++ b/Bugzilla/Product.pm @@ -34,6 +34,8 @@ use constant DEFAULT_CLASSIFICATION_ID => 1; #### Initialization #### ############################### +use constant IS_CONFIG => 1; + use constant DB_TABLE => 'products'; use constant DB_COLUMNS => qw( diff --git a/Bugzilla/Status.pm b/Bugzilla/Status.pm index 964df9070..1f8862a36 100644 --- a/Bugzilla/Status.pm +++ b/Bugzilla/Status.pm @@ -108,11 +108,21 @@ sub _check_value { sub BUG_STATE_OPEN { my $dbh = Bugzilla->dbh; - my $cache = Bugzilla->request_cache; - $cache->{status_bug_state_open} ||= - $dbh->selectcol_arrayref('SELECT value FROM bug_status - WHERE is_open = 1'); - return @{ $cache->{status_bug_state_open} }; + my $request_cache = Bugzilla->request_cache; + my $cache_key = 'status_bug_state_open'; + return @{ $request_cache->{$cache_key} } + if exists $request_cache->{$cache_key}; + + my $rows = Bugzilla->memcached->get_config({ key => $cache_key }); + if (!$rows) { + $rows = $dbh->selectcol_arrayref( + 'SELECT value FROM bug_status WHERE is_open = 1' + ); + Bugzilla->memcached->set_config({ key => $cache_key, data => $rows }); + } + + $request_cache->{$cache_key} = $rows; + return @$rows; } # Tells you whether or not the argument is a valid "open" state. diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 4f047ebdd..c37ab23f3 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -675,13 +675,23 @@ sub groups { FROM user_group_map WHERE user_id = ? AND isbless = 0}, undef, $self->id); - my $rows = $dbh->selectall_arrayref( - "SELECT DISTINCT grantor_id, member_id - FROM group_group_map - WHERE grant_type = " . GROUP_MEMBERSHIP); + my $cache_key = 'group_grant_type_' . GROUP_MEMBERSHIP; + my $membership_rows = Bugzilla->memcached->get_config({ + key => $cache_key, + }); + if (!$membership_rows) { + $membership_rows = $dbh->selectall_arrayref( + "SELECT DISTINCT grantor_id, member_id + FROM group_group_map + WHERE grant_type = " . GROUP_MEMBERSHIP); + Bugzilla->memcached->set_config({ + key => $cache_key, + data => $membership_rows, + }); + } my %group_membership; - foreach my $row (@$rows) { + foreach my $row (@$membership_rows) { my ($grantor_id, $member_id) = @$row; push (@{ $group_membership{$member_id} }, $grantor_id); } diff --git a/contrib/convert-workflow.pl b/contrib/convert-workflow.pl index fdcab977f..8f76dac7f 100755 --- a/contrib/convert-workflow.pl +++ b/contrib/convert-workflow.pl @@ -151,6 +151,7 @@ if ($enable_unconfirmed) { $dbh->do('UPDATE products SET allows_unconfirmed = 1'); } $dbh->bz_commit_transaction(); +Bugzilla->memcached->clear_all(); print <memcached->clear({ table => 'products', name => $name }); } + Bugzilla->memcached->clear_config(); LoadTemplate($action); } -- cgit v1.2.3-24-g4f1b