[SERVER-37283] View graph cycle on expressive lookup secondary read Created: 24/Sep/18  Updated: 29/Oct/23  Resolved: 11/Feb/19

Status: Closed
Project: Core Server
Component/s: Aggregation Framework
Affects Version/s: 4.0.0
Fix Version/s: 4.1.8

Type: Bug Priority: Major - P3
Reporter: James Wahlin Assignee: Charlie Swanson
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Backports
Depends
Problem/Incident
causes SERVER-47469 applyOps does not take exclusive lock... Closed
Related
related to SERVER-47123 Remove AutoGetOrCreateDb Closed
related to SERVER-38610 Use WithLock in ViewCatalog instead o... Closed
related to SERVER-56511 use AutoGetOrCreateCollection to crea... Closed
Backwards Compatibility: Fully Compatible
Operating System: ALL
Backport Requested:
v4.0
Steps To Reproduce:

The following patch is based on this commit and will make this issue easier to reproduce:

diff --git a/jstests/concurrency/fsm_workloads/view_catalog_cycle_lookup.js b/jstests/concurrency/fsm_workloads/view_catalog_cycle_lookup.js
index 4745ef3..cc1e14e 100644
--- a/jstests/concurrency/fsm_workloads/view_catalog_cycle_lookup.js
+++ b/jstests/concurrency/fsm_workloads/view_catalog_cycle_lookup.js
@@ -15,7 +15,7 @@ var $config = (function() {
     const prefix = 'view_catalog_cycle_lookup_';
 
     var data = {
-        viewList: ['viewA', 'viewB', 'viewC', 'viewD', 'viewE'].map(viewName => prefix + viewName),
+        viewList: ['viewA', 'viewB', 'viewC'].map(viewName => prefix + viewName),
         getRandomView: function(viewList) {
             return viewList[Random.randInt(viewList.length)];
         },
@@ -138,8 +138,8 @@ var $config = (function() {
     }
 
     return {
-        threadCount: 20,
-        iterations: 100,
+        threadCount: 60,
+        iterations: 150,
         data: data,
         states: states,
         startState: 'readFromView',
diff --git a/src/mongo/db/commands/run_aggregate.cpp b/src/mongo/db/commands/run_aggregate.cpp
index 958c33f..ee6b10c 100644
--- a/src/mongo/db/commands/run_aggregate.cpp
+++ b/src/mongo/db/commands/run_aggregate.cpp
@@ -244,6 +244,7 @@ StatusWith<StringMap<ExpressionContext::ResolvedNamespace>> resolveInvolvedNames
         } else if (viewCatalog->lookup(opCtx, involvedNs.ns())) {
             // If 'involvedNs' refers to a view namespace, then we resolve its definition.
             auto resolvedView = viewCatalog->resolveView(opCtx, involvedNs);
+            sleepmillis(100);
             if (!resolvedView.isOK()) {
                 return {ErrorCodes::FailedToParse,
                         str::stream() << "Failed to resolve view '" << involvedNs.ns() << "': " 

Run resmoke with the following arguments (this may take a few runs to trigger):

python buildscripts/resmoke.py --suite=concurrency_replication_causal_consistency jstests/concurrency/fsm_workloads/view_catalog_cycle_lookup.js

The following patch includes an additional change which will trigger an invariant if resolveInvolvedNamespaces() encounters a invalid ViewCatalog mid-resolution. This confirms that the MODE_IS lock is not protecting the ViewCatalog from change:

diff --git a/jstests/concurrency/fsm_workloads/view_catalog_cycle_lookup.js b/jstests/concurrency/fsm_workloads/view_catalog_cycle_lookup.js
index 4745ef3..cc1e14e 100644
--- a/jstests/concurrency/fsm_workloads/view_catalog_cycle_lookup.js
+++ b/jstests/concurrency/fsm_workloads/view_catalog_cycle_lookup.js
@@ -15,7 +15,7 @@ var $config = (function() {
     const prefix = 'view_catalog_cycle_lookup_';
 
     var data = {
-        viewList: ['viewA', 'viewB', 'viewC', 'viewD', 'viewE'].map(viewName => prefix + viewName),
+        viewList: ['viewA', 'viewB', 'viewC'].map(viewName => prefix + viewName),
         getRandomView: function(viewList) {
             return viewList[Random.randInt(viewList.length)];
         },
@@ -138,8 +138,8 @@ var $config = (function() {
     }
 
     return {
-        threadCount: 20,
-        iterations: 100,
+        threadCount: 60,
     return {
-        threadCount: 20,
-        iterations: 100,
+        threadCount: 60,
+        iterations: 150,
         data: data,
         states: states,
         startState: 'readFromView',
diff --git a/src/mongo/db/commands/run_aggregate.cpp b/src/mongo/db/commands/run_aggregate.cpp
index 958c33f..ea5530c 100644
--- a/src/mongo/db/commands/run_aggregate.cpp
+++ b/src/mongo/db/commands/run_aggregate.cpp
@@ -220,6 +220,7 @@ StatusWith<StringMap<ExpressionContext::ResolvedNamespace>> resolveInvolvedNames
     std::deque<NamespaceString> involvedNamespacesQueue(pipelineInvolvedNamespaces.begin(),
                                                         pipelineInvolvedNamespaces.end());
     StringMap<ExpressionContext::ResolvedNamespace> resolvedNamespaces;
+    bool allowViewCatalogReload = true;
 
     while (!involvedNamespacesQueue.empty()) {
         auto involvedNs = std::move(involvedNamespacesQueue.front());
@@ -243,7 +244,9 @@ StatusWith<StringMap<ExpressionContext::ResolvedNamespace>> resolveInvolvedNames
             resolvedNamespaces[involvedNs.coll()] = {involvedNs, std::vector<BSONObj>{}};
         } else if (viewCatalog->lookup(opCtx, involvedNs.ns())) {
             // If 'involvedNs' refers to a view namespace, then we resolve its definition.
-            auto resolvedView = viewCatalog->resolveView(opCtx, involvedNs);
+            auto resolvedView = viewCatalog->resolveView(opCtx, involvedNs, allowViewCatalogReload);
+            allowViewCatalogReload = false;
+            sleepmillis(100);
             if (!resolvedView.isOK()) {
                 return {ErrorCodes::FailedToParse,
                         str::stream() << "Failed to resolve view '" << involvedNs.ns() << "': "
diff --git a/src/mongo/db/views/view_catalog.cpp b/src/mongo/db/views/view_catalog.cpp
index eb1e6dc..e8bf1a4 100644
--- a/src/mongo/db/views/view_catalog.cpp
+++ b/src/mongo/db/views/view_catalog.cpp
@@ -430,7 +430,8 @@ std::shared_ptr<ViewDefinition> ViewCatalog::lookup(OperationContext* opCtx, Str
 }
 
 StatusWith<ResolvedView> ViewCatalog::resolveView(OperationContext* opCtx,
-                                                  const NamespaceString& nss) {
+                                                  const NamespaceString& nss,
+                                                  bool allowReload) {
     stdx::unique_lock<stdx::mutex> lock(_mutex);
 
     // Keep looping until the resolution completes. If the catalog is invalidated during the
@@ -461,6 +462,7 @@ StatusWith<ResolvedView> ViewCatalog::resolveView(OperationContext* opCtx,
 
             // If the catalog has been invalidated, bail and restart.
             if (!_valid.load()) {
+                invariant(allowReload);
                 uassertStatusOK(_reloadIfNeeded_inlock(opCtx));
                 break;
             }
diff --git a/src/mongo/db/views/view_catalog.h b/src/mongo/db/views/view_catalog.h
index 14bdf4d..d84245a 100644
--- a/src/mongo/db/views/view_catalog.h
+++ b/src/mongo/db/views/view_catalog.h
@@ -115,7 +115,9 @@ public:
      * fully-resolved view definition containing the backing namespace, the resolved pipeline and
      * the collation to use for the operation.
      */
-    StatusWith<ResolvedView> resolveView(OperationContext* opCtx, const NamespaceString& nss);
+    StatusWith<ResolvedView> resolveView(OperationContext* opCtx,
+                                         const NamespaceString& nss,
+                                         bool allowReload = true);
 
     /**
      * Reload the views catalog if marked invalid. No-op if already valid. Does only minimal 

Sprint: Query 2018-12-17, Query 2018-12-31, Query 2019-01-14, Query 2019-01-28, Query 2019-02-11, Query 2019-02-25
Participants:
Linked BF Score: 58

 Description   

It is possible for the view_catalog_cycle_lookup.js FSM test to fail with MaxSubPipelineDepthExceeded when run in a suite that performs secondary reads. This is caused when the aggregate command generates an invalid pipeline containing a view cycle and can happen when the view catalog changes while resolving pipeline namespaces.

When the aggregate command calls resolveInvolvedNamespaces() it holds a MODE_IS database lock which is meant to protect against ViewCatalog change, as collmod & collection drop/create require a database MODE_X lock. On secondaries however view catalog changes are replicated as inserts to the system.views collection and obtain a MODE_IX lock rather than MODE_X. This allows for view definition change mid-resolution and can result in an invalid view graph, one that may contain a cycle.



 Comments   
Comment by Githook User [ 11/Feb/19 ]

Author:

{'name': 'Charlie Swanson', 'email': 'charlie.swanson@mongodb.com', 'username': 'cswanson310'}

Message: SERVER-37283 Use stronger locks for system.views

Readers of the view catalog depend on a MODE_IS DB lock preventing
concurrent writes to the view catalog. This is true for regular view
maintenance commands like collMod, create, and drop. However, on
secondaries these commands are replicated as direct writes to
system.views and do not hold as strong of a lock. Further, a user is
permitted to write directly to system.views and so could hit a similar
issue on the primary.
Branch: master
https://github.com/mongodb/mongo/commit/d568e329a67eee8ba241d52067750a3d8c42dc0f

Comment by Charlie Swanson [ 11/Dec/18 ]

This got quite complicated quite quickly. Pausing on this for now in favor of other work.

Comment by Martin Neupauer [ 09/Nov/18 ]

The issue lays in resolveInvolvedNamespaces in run_aggregate.cpp. There is a while loop (https://github.com/mongodb/mongo/blob/1c2b3f3ad137758d6cc6275a61841b0836095d6b/src/mongo/db/commands/run_aggregate.cpp#L226) that repeatedly calls viewCatalog->resolveView and viewCatalog->lookup.
While these calls are thread safe in respect to calls from other threads they do not provide a stable "snapshot" of viewCatalog; i.e. the viewCatalog internal state is free to change in between the calls.
One approach how to fix this would be to "lock down" the view Catalog for the duration of resolveInvolvedNamespaces. We could replace the mutex inside the viewCatalog with recursive_mutex and lock the catalog upon entry to resolveInvolvedNamespaces.
Potential risks are:
1. decreased concurrency and the catalog could be inaccessible for longer than before.
2. deadlocks (maybe as they have a tendency to always show up whenever there is a change in the concurrency.

Another approach is to make sure we take the same database/collection locks on secondaries as we do primaries as this problem does not exist on primaries.

Comment by Ian Whalen (Inactive) [ 06/Nov/18 ]

martin.neupauer have you come up with any additional info you can add here in the past few BF Fridays? Ticket has been in investigating with no update for ~1 month so I'd like to get it moving forward.

Comment by David Storch [ 04/Oct/18 ]

@martin.neupauer to use BF Friday to investigate how to fix this.

Comment by James Wahlin [ 25/Sep/18 ]

Updated. The only released version this affects is 4.0. Prior to SERVER-34192 the aggregate command's MODE_IS lock would have blocked modification of the system.views collection.

Comment by David Storch [ 25/Sep/18 ]

james.wahlin, can you fill out the "affects versions" field? It sounds like this issue has been present since views were first implemented in 3.4?

Generated at Thu Feb 08 04:45:32 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.