SERVER-5353 Backport print capped collection scan on _id query warning message in appropriate cases.

5 files changed, 118 insertions, 18 deletions

diff --git a/db/queryoptimizer.cpp b/db/queryoptimizer.cpp
index 71ca6576fc4..d91b79aac12 100644
--- a/db/queryoptimizer.cpp
+++ b/db/queryoptimizer.cpp
@@ -214,22 +214,6 @@ doneCheckOrder:
         if ( willScanTable() ) {
             if ( _frs.nNontrivialRanges() ) {
                 checkTableScanAllowed( _frs.ns() );
-                
-                // if we are doing a table scan on _id
-                // and its a capped collection
-                // we disallow as its a common user error
-                // .system. and local collections are exempt
-                if ( _d && _d->capped && _frs.range( "_id" ).nontrivial() ) {
-                    if ( cc().isSyncThread() ||
-                         str::contains( _frs.ns() , ".system." ) || 
-                         str::startsWith( _frs.ns() , "local." ) ) {
-                        // ok
-                    }
-                    else {
-                        warning() << "_id query on capped collection without an _id index, performance will be poor collection: " << _frs.ns() << endl;
-                        //uassert( 14820, str::stream() << "doing _id query on a capped collection without an index is not allowed: " << _frs.ns() ,
-                    }
-                }
             }
             return findTableScan( _frs.ns(), _order, startLoc );
         }
@@ -486,12 +470,14 @@ doneCheckOrder:
                     _usingPrerecordedPlan = true;
                     _mayRecordPlan = false;
                     _plans.push_back( p );
+                    warnOnCappedIdTableScan();
                     return;
                 }
             }
         }
 
         addOtherPlans( false );
+        warnOnCappedIdTableScan();
     }
 
     void QueryPlanSet::addOtherPlans( bool checkFirst ) {
@@ -633,6 +619,31 @@ doneCheckOrder:
         }
         return _plans[0];
     }
+    
+    void QueryPlanSet::warnOnCappedIdTableScan() const {
+        // if we are doing a table scan on _id
+        // and it's a capped collection
+        // we warn as it's a common user error
+        // .system. and local collections are exempt
+        const char *ns = _frsp->ns();
+        NamespaceDetails *d = nsdetails( ns );
+        if ( d &&
+            d->capped &&
+            nPlans() == 1 &&
+            firstPlan()->willScanTable() &&
+            firstPlan()->multikeyFrs().range( "_id" ).nontrivial() ) {
+            if ( cc().isSyncThread() ||
+                str::contains( ns , ".system." ) ||
+                str::startsWith( ns , "local." ) ) {
+                // ok
+            }
+            else {
+                warning()
+                << "unindexed _id query on capped collection, "
+                << "performance will be poor collection: " << ns << endl;
+            }
+        }
+    }
 
     QueryPlanSet::Runner::Runner( QueryPlanSet &plans, QueryOp &op ) :
         _op( op ),
diff --git a/db/queryoptimizer.h b/db/queryoptimizer.h
index fea6c0b82ae..78d169d7e1c 100644
--- a/db/queryoptimizer.h
+++ b/db/queryoptimizer.h
@@ -314,6 +314,7 @@ namespace mongo {
         }
         void init();
         void addHint( IndexDetails &id );
+        void warnOnCappedIdTableScan() const;
         class Runner {
         public:
             Runner( QueryPlanSet &plans, QueryOp &op );
diff --git a/jstests/queryoptimizera.js b/jstests/queryoptimizera.js
new file mode 100644
index 00000000000..48d7ccf6180
--- /dev/null
+++ b/jstests/queryoptimizera.js
@@ -0,0 +1,87 @@
+// Check that a warning message about doing a capped collection scan for a query with an _id
+// constraint is printed at appropriate times.  SERVER-5353
+
+function numWarnings() {
+    logs = db.adminCommand( { getLog:"global" } ).log
+    ret = 0;
+    logs.forEach( function( x ) {
+                 if ( x.match( warningMatchRegexp ) ) {
+                 ++ret;
+                 }
+                 } );
+    return ret;
+}
+
+collectionNameIndex = 0;
+
+// Generate a collection name not already present in the log.
+do {
+    testCollectionName = 'jstests_queryoptimizera__' + collectionNameIndex++;
+    warningMatchString = 'unindexed _id query on capped collection.*collection: test.' +
+        testCollectionName;
+    warningMatchRegexp = new RegExp( warningMatchString );    
+    
+} while( numWarnings() > 0 );
+
+t = db[ testCollectionName ];
+t.drop();
+
+notCappedCollectionName = testCollectionName + '_notCapped';
+
+notCapped = db[ notCappedCollectionName ];
+notCapped.drop();
+
+db.createCollection( testCollectionName, { capped:true, size:1000 } );
+db.createCollection( notCappedCollectionName, { autoIndexId:false } );
+
+t.insert( {} );
+notCapped.insert( {} );
+
+oldNumWarnings = 0;
+
+function assertNoNewWarnings() {
+    assert.eq( oldNumWarnings, numWarnings() );
+}
+
+function assertNewWarning() {
+    ++oldNumWarnings;
+    assert.eq( oldNumWarnings, numWarnings() );
+}
+
+// Simple _id query without an _id index.
+t.find( { _id:0 } ).itcount();
+assertNewWarning();
+
+// Simple _id query without an _id index, on a non capped collection.
+notCapped.find( { _id:0 } ).itcount();
+assertNoNewWarnings();
+
+// A multi field query, including _id.
+t.find( { _id:0, a:0 } ).itcount();
+assertNewWarning();
+
+// An unsatisfiable query.
+t.find( { _id:0, a:{$in:[]} } ).itcount();
+assertNoNewWarnings();
+
+// An hinted query.
+t.find( { _id:0 } ).hint( { $natural:1 } ).itcount();
+assertNoNewWarnings();
+
+// Retry a multi field query.
+t.find( { _id:0, a:0 } ).itcount();
+assertNewWarning();
+
+// Warnings should not be printed when an index is added on _id.
+t.ensureIndex( { _id:1 } );
+
+t.find( { _id:0 } ).itcount();
+assertNoNewWarnings();
+
+t.find( { _id:0, a:0 } ).itcount();
+assertNoNewWarnings();
+
+t.find( { _id:0, a:0 } ).itcount();
+assertNoNewWarnings();
+
+t.drop(); // cleanup
diff --git a/jstests/slowNightly/sharding_passthrough.js b/jstests/slowNightly/sharding_passthrough.js
index d81df685bc5..fa1f7bc9a99 100644
--- a/jstests/slowNightly/sharding_passthrough.js
+++ b/jstests/slowNightly/sharding_passthrough.js
@@ -72,7 +72,7 @@ files.forEach(
 	    return;
 	}
 	// These aren't supposed to get run under sharding:
-	if (/[\/\\](dbadmin|error1|fsync|fsync2|geo.*|indexh|remove5|update4|notablescan|compact.*|check_shard_index|bench_test.*|mr_replaceIntoDB)\.js$/.test(x.name)) {
+	if (/[\/\\](dbadmin|error1|fsync|fsync2|geo.*|indexh|remove5|update4|notablescan|compact.*|check_shard_index|bench_test.*|mr_replaceIntoDB|queryoptimizera)\.js$/.test(x.name)) {
 	    print(" >>>>>>>>>>>>>>> skipping test that would fail under sharding " + x.name)	    
 	    return;
 	}
diff --git a/shell/utils.js b/shell/utils.js
index 7d7a23b6063..4e206034d2a 100644
--- a/shell/utils.js
+++ b/shell/utils.js
@@ -739,7 +739,8 @@ if ( typeof _threadInject != "undefined" ){
                                    "jstests/notablescan.js",
                                    "jstests/drop2.js",
                                    "jstests/dropdb_race.js",
-                                   "jstests/bench_test1.js"] );
+                                   "jstests/bench_test1.js",
+                                   "jstests/queryoptimizera.js"] );
         
         // some tests can't be run in parallel with each other
         var serialTestsArr = [ "jstests/fsync.js",