[SERVER-36564] Reconsider use of StubMongoProcessInterface when ViewCatalog validates a view pipeline Created: 09/Aug/18  Updated: 19/Sep/18  Resolved: 19/Sep/18

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

Type: Bug Priority: Major - P3
Reporter: Kyle Suarez Assignee: Nicholas Zolnierz
Resolution: Won't Fix Votes: 0
Labels: read-only-views
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
is related to SERVER-36123 Reject $out with mode: "replaceCollec... Closed
Operating System: ALL
Sprint: Query 2018-09-24
Participants:

 Description   

When creating or modifying a view, the ViewCatalog will parse the view pipeline for validation. Doing so requires a MongoProcessInterface, and the ViewCatalog uses the stub implementation:

view_catalog.cpp

246
    boost::intrusive_ptr<ExpressionContext> expCtx =
247
        new ExpressionContext(opCtx,
248
                              request,
249
                              CollatorInterface::cloneCollator(viewDef.defaultCollator()),
250
                              // We can use a stub MongoProcessInterface because we are only parsing
251
                              // the Pipeline for validation here. We won't do anything with the
252
                              // pipeline that will require a real implementation.
253
                              std::make_shared<StubMongoProcessInterface>(),
254
                              std::move(resolvedNamespaces),
255
                              boost::none);
256
 
257
    // Save this to a variable to avoid reading the atomic variable multiple times.
258
    auto currentFCV = serverGlobalParams.featureCompatibility.getVersion();
259
 
260
    // If the feature compatibility version is not 4.2, and we are validating features as master,
261
    // ban the use of new agg features introduced in 4.2 to prevent them from being persisted in the
262
    // catalog.
263
    if (serverGlobalParams.validateFeaturesAsMaster.load() &&
264
        currentFCV != ServerGlobalParams::FeatureCompatibility::Version::kFullyUpgradedTo42) {
265
        expCtx->maxFeatureCompatibilityVersion = currentFCV;
266
    }
267
    auto pipelineStatus = Pipeline::parse(viewDef.pipeline(), std::move(expCtx));
268
    if (!pipelineStatus.isOK()) {
269
        return pipelineStatus.getStatus();
270
    }

But with the new $out improvements in 4.1 to output to a sharded collection, $out parsing requires a valid implementation of MongoProcessInterface::isSharded(). We should revisit this code to prepare for a world where $out and $lookup might require a real MongoProcessInterface for parsing.



 Comments   
Comment by Charlie Swanson [ 19/Sep/18 ]

Closing as won't fix based on the discussion above.

Comment by David Storch [ 19/Sep/18 ]

charlie.swanson I agree, let's close this ticket.

Comment by Charlie Swanson [ 19/Sep/18 ]

This sounds fine to me - I don't see any real risk here in the near-term. I think we likely could spend some time working on the library graph and doing some sort of 'pImpl' trick to solve the dependency problem, but this doesn't seem particularly high-value.

david.storch Ok to close out? It'd be nice to do so before sprint planning tomorrow if so.

Comment by Nicholas Zolnierz [ 18/Sep/18 ]

I don't think a "fix" for this is worth pursuing at this time, for the following reasons:

1. Switching to a LiteParsedPipeline would obviate the need for a MongoProcessInterface, however we would then miss the FCV checks for using features that aren't supported. I didn't test this but if we did do this then the view creation would succeed but subsequent reads would fail.
2. Using a real MongoProcessInterface is not trivial, since the view catalog code is compiled into both mongos and mongod. It would likely require some refactoring to separate out the view creation.

Given that this ticket is meant to prevent speculative bugs in the future, I think it makes sense to drop for now. david.storch, what do you think?

cc charlie.swanson

Comment by David Storch [ 13/Sep/18 ]

No, I'll slot it into your regular sprint.

Comment by Nicholas Zolnierz [ 13/Sep/18 ]

david.storch yeah that works too, do you want to consider SERVER-37108 part of BFF?

Comment by David Storch [ 13/Sep/18 ]

kyle.suarez nicholas.zolnierz, I think Nick could also pick upĀ SERVER-37108?

Comment by Kyle Suarez [ 12/Sep/18 ]

Pulling this into the current sprint because Nick's out of work. I don't think there are other "critical path"/blocker tickets for the $out project, so this feels like an appropriate next step.

Comment by Ian Whalen (Inactive) [ 24/Aug/18 ]

Adding to the $out project - this will probably get done as part of that effort or just resolved as won't fix.

Comment by Nicholas Zolnierz [ 09/Aug/18 ]

Hmm I wonder if we could get by with just a lite parsed pipeline instead of a full parse?

Comment by Kyle Suarez [ 09/Aug/18 ]

CC nicholas.zolnierz

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