[Pkg-puppet-devel] [SCM] Puppet packaging for Debian branch, upstream, updated. puppet-0.24.5-rc3-1456-g2f0b1e5

[James Turnbull]

The following commit has been merged in the upstream branch:
commit 2b57e065d2220be4f172ae429190bd116ddbdaf1
Author: Brice Figureau <brice-puppet at daysofwonder.com>
Date:   Thu Oct 22 20:09:27 2009 +0200

    Fix #2691 - Collection AR request should not include params if querying with tags
    
    f9516d introduced a change in the way the user tags are persisted
    to the database: user tags are now treated as regular tags (they are
    stored to the tags table).
    Thus this commit changed the AR collector request to also look at the
    tags tables when collecting.
    
    Unfortunately this added a performance regression since tag request
    were still importing the resources parameters tables and AR was
    issuing a large request which was returning all the resource parameters
    joined with the tags.
    
    This commit fixes the AR request to join to the needed table, instead
    of doing an include. Including (ie eager loading) parameter values was
    not working for resource parameters anyway since at least 0.24 because
    searching by parameter add a constraint to the joins and only the
    searched parameter was returned instead of all parameter for a given
    exported resource. So on a performance standpoint this new code should
    be as fast 0.24 was.
    
    Signed-off-by: Brice Figureau <brice-puppet at daysofwonder.com>

diff --git a/lib/puppet/parser/collector.rb b/lib/puppet/parser/collector.rb
index a7f81b4..a6763c4 100644
--- a/lib/puppet/parser/collector.rb
+++ b/lib/puppet/parser/collector.rb
@@ -102,18 +102,24 @@ class Puppet::Parser::Collector
         raise Puppet::DevError, "Cannot collect resources for a nil host" unless @scope.host
         host = Puppet::Rails::Host.find_by_name(@scope.host)
 
-        query = {:include => {:param_values => :param_name}}
-
         search = "(exported=? AND restype=?)"
         values = [true, @type]
 
         search += " AND (%s)" % @equery if @equery
 
-        # this is a small performance optimisation
-        # the tag mechanism involves 3 more joins, which are
-        # needed only if we query on tags.
-        if search =~ /puppet_tags/
-            query[:include][:puppet_tags] = :resource_tags
+        # note:
+        # we're not eagerly including any relations here because
+        # it can creates so much objects we'll throw out later.
+        # We used to eagerly include param_names/values but the way
+        # the search filter is built ruined those efforts and we
+        # were eagerly loading only the searched parameter and not
+        # the other ones. 
+        query = {}
+        case search
+        when /puppet_tags/
+            query = {:joins => {:resource_tags => :puppet_tag}}
+        when /param_name/
+            query = {:joins => {:param_values => :param_name}}
         end
 
         # We're going to collect objects from rails, but we don't want any
@@ -139,7 +145,7 @@ class Puppet::Parser::Collector
         # and such, we'll need to vary the conditions, but this works with no
         # attributes, anyway.
         time = Puppet::Util.thinmark do
-            Puppet::Rails::Resource.find(:all, @type, true, query).each do |obj|
+            Puppet::Rails::Resource.find(:all, query).each do |obj|
                 if resource = exported_resource(obj)
                     count += 1
                     resources << resource
diff --git a/spec/unit/parser/collector.rb b/spec/unit/parser/collector.rb
index 926033c..7f88bf7 100755
--- a/spec/unit/parser/collector.rb
+++ b/spec/unit/parser/collector.rb
@@ -498,35 +498,46 @@ describe Puppet::Parser::Collector, "when building its ActiveRecord query for co
         Puppet::Rails::Host.expects(:find_by_name).with(@scope.host).returns(@host)
 
         Puppet::Rails::Resource.stubs(:find).with { |*arguments|
-            options = arguments[3]
+            options = arguments[1]
             options[:conditions][0] =~ /^host_id != \?/ and options[:conditions][1] == 5
         }.returns([@resource])
 
         @collector.evaluate.should == [@resource]
     end
 
-    it "should return parameter names, parameter values when querying ActiveRecord" do
+    it "should join with parameter names, parameter values when querying ActiveRecord" do
+        @collector.equery = "param_names.name = title"
         Puppet::Rails::Resource.stubs(:find).with { |*arguments|
-            options = arguments[3]
-            options[:include] == {:param_values => :param_name}
+            options = arguments[1]
+            options[:joins] == {:param_values => :param_name}
         }.returns([@resource])
 
         @collector.evaluate.should == [@resource]
     end
 
-    it "should return tags when querying ActiveRecord with a tag exported query" do
+    it "should join with tag tables when querying ActiveRecord with a tag exported query" do
         @collector.equery = "puppet_tags.name = test"
         Puppet::Rails::Resource.stubs(:find).with { |*arguments|
-            options = arguments[3]
-            options[:include] == {:param_values => :param_name, :puppet_tags => :resource_tags}
+            options = arguments[1]
+            options[:joins] == {:resource_tags => :puppet_tag}
         }.returns([@resource])
 
         @collector.evaluate.should == [@resource]
     end
 
+    it "should not join parameters when querying ActiveRecord with a tag exported query" do
+        @collector.equery = "puppet_tags.name = test"
+        Puppet::Rails::Resource.stubs(:find).with { |*arguments|
+            options = arguments[1]
+            options[:joins] == {:param_values => :param_name}
+        }.returns([@resource])
+
+        @collector.evaluate.should be_false
+    end
+
     it "should only search for exported resources with the matching type" do
         Puppet::Rails::Resource.stubs(:find).with { |*arguments|
-            options = arguments[3]
+            options = arguments[1]
             options[:conditions][0].include?("(exported=? AND restype=?)") and options[:conditions][1] == true and options[:conditions][2] == "Mytype"
         }.returns([@resource])
 
@@ -536,7 +547,7 @@ describe Puppet::Parser::Collector, "when building its ActiveRecord query for co
     it "should include the export query if one is provided" do
         @collector.equery = "test = true"
         Puppet::Rails::Resource.stubs(:find).with { |*arguments|
-            options = arguments[3]
+            options = arguments[1]
             options[:conditions][0].include?("test = true")
         }.returns([@resource])
 

-- 
Puppet packaging for Debian