[SERVER-66355] Pass dbName to validateViewDefinitionBSON in DurableViewCatalog::onExternalInsert Created: 10/May/22  Updated: 29/Oct/23  Resolved: 08/Jun/22

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 6.1.0-rc0, 6.0.5

Type: Task Priority: Major - P3
Reporter: Janna Golden Assignee: Dianna Hohensee (Inactive)
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Backports
Depends
is depended on by SERVER-66286 DurableViewCatalog to convert from St... Closed
Problem/Incident
is caused by SERVER-53870 Improve view creation performance ove... Closed
Backwards Compatibility: Fully Compatible
Sprint: Execution Team 2022-06-13
Participants:
Linked BF Score: 10

 Description   

validateViewDefinitionBSON is meant to take in the db name, however in DurableViewCatalog::onExternalInsert we pass the full namespace. This causes this uassert to fail because database names are not allowed to contain a ".", and since we pass the full namespace for dbName, it will contain a ".". The exception is currently swallowed because we catch exceptions thrown by onExternalInsert.

However, if we change DurableViewCatalog::onExternalInsert to correctly pass the dbName, we'll eventually hit this invariant in CollectionCatalog::createView because we don't actually ever take a collection lock on the view name currently - we have a lock on the database and the collection name itself, but not the view.



 Comments   
Comment by Githook User [ 25/Jan/23 ]

Author:

{'name': 'Dianna Hohensee', 'email': 'dianna.hohensee@mongodb.com', 'username': 'DiannaHohensee'}

Message: SERVER-66355 Fix DurableViewCatalog bug where nss was passed into dbName parameter; Don't require view namespace lock during oplog applicaion of db.system.views inserts

(cherry picked from commit 1a578ef55d316eb5fec5cc135cf056f6f17ab616)
Branch: v6.0
https://github.com/mongodb/mongo/commit/792cdd0ead386ebb7f278d973e8fa6c14a0053dc

Comment by Githook User [ 08/Jun/22 ]

Author:

{'name': 'Dianna Hohensee', 'email': 'dianna.hohensee@mongodb.com', 'username': 'DiannaHohensee'}

Message: SERVER-66355 Fix DurableViewCatalog bug where nss was passed into dbName parameter; Don't require view namespace lock during oplog applicaion of db.system.views inserts
Branch: master
https://github.com/mongodb/mongo/commit/1a578ef55d316eb5fec5cc135cf056f6f17ab616

Comment by Dianna Hohensee (Inactive) [ 02/Jun/22 ]

 I got a stack trace of the invariant. Looks like oplog application.

[ReplWriterWorker-0] "Invariant failure","attr":{"expr":"opCtx->lockState()->isCollectionLockedForMode(viewName, MODE_IX)","file":"src/mongo/db/catalog/collection_catalog.cpp","line":479}
[ReplWriterWorker-0] "\n\n***aborting after invariant() failure\n\n"
[ReplWriterWorker-0] "Writing fatal message","attr":{"message":"Got signal: 6 (Aborted).\n"}
....
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F7770133078","b":"7F776FF3F000","o":"1F4078","s":"_ZN5mongo18stack_trace_detail12_GLOBAL__N_119printStackTraceImplERKNS1_7OptionsEPNS_14StackTraceSinkE.constprop.362","C":"mongo::stack_trace_detail::(anonymous namespace)::printStackTraceImpl(mongo::stack_trace_detail::(anonymous namespace)::Options const&, mongo::StackTraceSink*) [clone .constprop.362]","s+":"1F8"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F7770135599","b":"7F776FF3F000","o":"1F6599","s":"_ZN5mongo15printStackTraceEv","C":"mongo::printStackTrace()","s+":"29"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F777012F2DF","b":"7F776FF3F000","o":"1F02DF","s":"abruptQuit","s+":"6F"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F776E421D80","b":"7F776E40F000","o":"12D80","s":"funlockfile","s+":"50"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F776E08293F","b":"7F776E04B000","o":"3793F","s":"gsignal","s+":"10F"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F776E06CC95","b":"7F776E04B000","o":"21C95","s":"abort","s+":"127"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F777006DF4B","b":"7F776FF3F000","o":"12EF4B","s":"_ZN5mongo12_GLOBAL__N_19callAbortEv","C":"mongo::(anonymous namespace)::callAbort()","s+":"1B"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F777006EE1A","b":"7F776FF3F000","o":"12FE1A","s":"_ZN5mongo15invariantFailedEPKcS1_j","C":"mongo::invariantFailed(char const*, char const*, unsigned int)","s+":"EC"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F77668B9714","b":"7F776689F000","o":"1A714","s":"_ZNK5mongo17CollectionCatalog10createViewEPNS_16OperationContextERKNS_15NamespaceStringES5_RKNS_9BSONArrayERKNS_7BSONObjERKSt8functionIFNS_10StatusWithIN4absl12lts_2021032413node_hash_setIS3_NSF_13hash_internal4HashIS3_EESt8equal_toIS3_ESaIS3_EEEEES2_RKNS_14ViewDefinitionEEEb.cold.1967","C":"mongo::CollectionCatalog::createView(mongo::OperationContext*, mongo::NamespaceString const&, mongo::NamespaceString const&, mongo::BSONArray const&, mongo::BSONObj const&, std::function<mongo::StatusWith<absl::lts_20210324::node_hash_set<mongo::NamespaceString, absl::lts_20210324::hash_internal::Hash<mongo::NamespaceString>, std::equal_to<mongo::NamespaceString>, std::allocator<mongo::NamespaceString> > > (mongo::OperationContext*, mongo::ViewDefinition const&)> const&, bool) const [clone .cold.1967]","s+":"6C"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F776CA796D6","b":"7F776CA6D000","o":"C6D6","s":"_ZN5mongo18DurableViewCatalog16onExternalInsertEPNS_16OperationContextERKNS_7BSONObjERKNS_15NamespaceStringE","C":"mongo::DurableViewCatalog::onExternalInsert(mongo::OperationContext*, mongo::BSONObj const&, mongo::NamespaceString const&)","s+":"596"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F776CAA8919","b":"7F776CA86000","o":"22919","s":"_ZN5mongo14OpObserverImpl9onInsertsEPNS_16OperationContextERKNS_15NamespaceStringERKNS_4UUIDEN9__gnu_cxx17__normal_iteratorIPKNS_15InsertStatementESt6vectorISB_SaISB_EEEESH_b","C":"mongo::OpObserverImpl::onInserts(mongo::OperationContext*, mongo::NamespaceString const&, mongo::UUID const&, __gnu_cxx::__normal_iterator<mongo::InsertStatement const*, std::vector<mongo::InsertStatement, std::allocator<mongo::InsertStatement> > >, __gnu_cxx::__normal_iterator<mongo::InsertStatement const*, std::vector<mongo::InsertStatement, std::allocator<mongo::InsertStatement> > >, bool)","s+":"849"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F77705CB4E1","b":"7F7770592000","o":"394E1","s":"_ZN5mongo18OpObserverRegistry9onInsertsEPNS_16OperationContextERKNS_15NamespaceStringERKNS_4UUIDEN9__gnu_cxx17__normal_iteratorIPKNS_15InsertStatementESt6vectorISB_SaISB_EEEESH_b","C":"mongo::OpObserverRegistry::onInserts(mongo::OperationContext*, mongo::NamespaceString const&, mongo::UUID const&, __gnu_cxx::__normal_iterator<mongo::InsertStatement const*, std::vector<mongo::InsertStatement, std::allocator<mongo::InsertStatement> > >, __gnu_cxx::__normal_iterator<mongo::InsertStatement const*, std::vector<mongo::InsertStatement, std::allocator<mongo::InsertStatement> > >, bool)","s+":"81"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F7766DDCCC1","b":"7F7766D7B000","o":"61CC1","s":"_ZNK5mongo14CollectionImpl27insertDocumentForBulkLoaderEPNS_16OperationContextERKNS_7BSONObjERKSt8functionIFNS_6StatusERKNS_8RecordIdEEE","C":"mongo::CollectionImpl::insertDocumentForBulkLoader(mongo::OperationContext*, mongo::BSONObj const&, std::function<mongo::Status (mongo::RecordId const&)> const&) const","s+":"521"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F776C4BF3EC","b":"7F776C489000","o":"363EC","s":"_ZN5mongo4repl24CollectionBulkLoaderImpl37_insertDocumentsForUncappedCollectionEN9__gnu_cxx17__normal_iteratorIPKNS_7BSONObjESt6vectorIS4_SaIS4_EEEESA_","C":"mongo::repl::CollectionBulkLoaderImpl::_insertDocumentsForUncappedCollection(__gnu_cxx::__normal_iterator<mongo::BSONObj const*, std::vector<mongo::BSONObj, std::allocator<mongo::BSONObj> > >, __gnu_cxx::__normal_iterator<mongo::BSONObj const*, std::vector<mongo::BSONObj, std::allocator<mongo::BSONObj> > >)","s+":"25C"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F776C4C0BFA","b":"7F776C489000","o":"37BFA","s":"_ZN5mongo4repl24CollectionBulkLoaderImpl15insertDocumentsEN9__gnu_cxx17__normal_iteratorIPKNS_7BSONObjESt6vectorIS4_SaIS4_EEEESA_","C":"mongo::repl::CollectionBulkLoaderImpl::insertDocuments(__gnu_cxx::__normal_iterator<mongo::BSONObj const*, std::vector<mongo::BSONObj, std::allocator<mongo::BSONObj> > >, __gnu_cxx::__normal_iterator<mongo::BSONObj const*, std::vector<mongo::BSONObj, std::allocator<mongo::BSONObj> > >)","s+":"7A"}}
[ReplWriterWorker-0] "Frame","attr":{"frame":{"a":"7F776BC0A43E","b":"7F776BBE0000","o":"2A43E","s":"_ZN5mongo4repl16CollectionCloner23insertDocumentsCallbackERKNS_8executor12TaskExecutor12CallbackArgsE","C":"mongo::repl::CollectionCloner::insertDocumentsCallback(mongo::executor::TaskExecutor::CallbackArgs const&)","s+":"11E"}}

Comment by Janna Golden [ 01/Jun/22 ]

The code catches the error and retries down a different path - it'll reload the views in the collection catalog rather than creating the view. I think in either case the view will exist in the collection catalog in the end, the code used to always reload the views. Looking at the ticket (SERVER-53870) that added DurableViewCatalog::onExternalInsert, it looks this function was added as a performance optimization to avoid reloading the views. I don't know that there's a great correctness test we could write because I think the outcome is the same either way, but perhaps you could run the script that's in SERVER-53870 to confirm the change?

We found this when making changes to swap out some uses of StringData or std::string for passing a db name around to use DatabaseName because we had attempted to change this line to grab the DatabaseName object from the namespace string instead, and that caused us to the invariant in CollectionCatalog::createView that we have a lock on view name (we don't).

EDIT - we have a ticket to make that change, and I've linked as depends on here.

Generated at Thu Feb 08 06:05:11 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.