[SERVER-16889] Query subsystem public API should use std::unique_ptr for ownership transfer Created: 16/Jan/15  Updated: 05/Feb/16  Resolved: 06/Jul/15

Status: Closed
Project: Core Server
Component/s: Querying
Affects Version/s: None
Fix Version/s: 3.1.6

Type: Improvement Priority: Major - P3
Reporter: J Rassi Assignee: Qingyang Chen
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on SERVER-16559 Require C++11 when building the server Closed
Duplicate
is duplicated by SERVER-18068 Coverity analysis defect 72413: Resou... Closed
Related
related to SERVER-14747 memory leak in the MultiPlanStage Closed
related to SERVER-15485 CanonicalQuery::canonicalize can leak... Closed
related to SERVER-15639 Text queries can return incorrect res... Closed
related to SERVER-15758 Memory leak when running find_dedup.j... Closed
related to SERVER-16035 Leaks emanating from QueryPlannerAcce... Closed
is related to SERVER-14892 Invalid {$elemMatch: {$where}} query ... Closed
is related to SERVER-17901 Memory leak in IndexCatalog::_isSpecOk() Closed
is related to SERVER-19040 WhereMatchExpression::shallowClone ca... Closed
Backwards Compatibility: Fully Compatible
Sprint: Quint Iteration 6
Participants:

 Description   

The main entry methods in the query subsystem transfer ownership of heap-allocated objects through raw pointers, which is error-prone. These methods should use std::unique_ptr instead. Partial list follows:

  • CanonicalQuery::canonicalize(). Caller receives ownership of a CanonicalQuery.
  • LiteParsedQuery::make(). Caller receives ownership of a LiteParsedQuery.
  • StatusWithMatchExpression replace MatchExpression* with unique_ptr<MatchExpression>
  • MatchExpression::shallowClone(). Caller receives ownership of a MatchExpression.
  • PlanExecutor::make(). Caller releases ownership of a WorkingSet, a PlanStage, a CanonicalQuery and a QuerySolution, and receives ownership of a PlanExecutor.
  • getExecutor(). Caller releases ownership of a CanonicalQuery, and receives ownership of a PlanExecutor (other public methods in get_executor.cpp can follow suit).
  • PlanExecutor::getStats(). Caller receives ownership of a PlanStageStats.

Linking a few tickets describing ownership-related memory leaks induced by incorrect use of the above (and related) methods.



 Comments   
Comment by Githook User [ 06/Jul/15 ]

Author:

{u'username': u'dannenberg', u'name': u'matt dannenberg', u'email': u'matt.dannenberg@10gen.com'}

Message: SERVER-16889 SERVER-19240 fix style
Branch: master
https://github.com/mongodb/mongo/commit/6e7683cb836aa08b6df9aa2738774015236cd126

Comment by Githook User [ 06/Jul/15 ]

Author:

{u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}

Message: SERVER-16889 StatusWithMatchExpression replace ME* with unique_ptr<ME>
Branch: master
https://github.com/mongodb/mongo/commit/32f04880f175fd3a3ce848ca074be72adc6d638c

Comment by Githook User [ 06/Jul/15 ]

Author:

{u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}

Message: SERVER-16889 StatusWithMatchExpression replace ME* with unique_ptr<ME>
Branch: master
https://github.com/10gen/mongo-enterprise-modules/commit/e5c3738bec5b7e45592c87898588246e799877a3

Comment by Githook User [ 29/Jun/15 ]

Author:

{u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}

Message: SERVER-16889.5 PlanExecutor::getStats() and PlanStage::getStats() return unique_ptr
Branch: master
https://github.com/mongodb/mongo/commit/9d72fd0f2f18b66423b9107e30cf2792656d2eab

Comment by Githook User [ 26/Jun/15 ]

Author:

{u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}

Message: SERVER-16889 MatchExpression::shallowClone() return unique_ptr<ME>
Branch: master
https://github.com/mongodb/mongo/commit/badc2ac4496c47ae2bbdebf47888a1a5449a22a1

Comment by J Rassi [ 26/Jun/15 ]

No, let's not consider those work items part of the scope of this ticket. Though there's plenty more work that needs to be done for the query subsystem in this area, the work already outlined is going to yield us a large benefit, and there's higher-impact work to schedule instead.

Comment by Qingyang Chen [ 26/Jun/15 ]

Should we also do:

  • QueryPlannerAccess:: (all the static QuerySolutionNode* functions)
  • PlanCache:: (all the functions that take ownership of parameters or has an out-parameter)
  • prepareExecution()
Comment by Githook User [ 26/Jun/15 ]

Author:

{u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}

Message: SERVER-16889 Modernize getExecutor*(), PlanExecutor::make() signatures
Branch: master
https://github.com/mongodb/mongo/commit/159f54fcb550d6ff660efd2832ac5ae8b6fced56

Comment by Githook User [ 23/Jun/15 ]

Author:

{u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}

Message: SERVER-16889 CanonicalQuery::canonicalize() return SW<unique_ptr<CQ>>
Branch: master
https://github.com/mongodb/mongo/commit/d674d15ee5cb573567d0683041c8049c8ee8cb21

Comment by Githook User [ 22/Jun/15 ]

Reverting due to Code Freeze, will re-push after Code Freeze is lifted.

Author:

{u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}

Message: Revert "SERVER-16889 query subsystem CanonicalQuery::canonicalize use StatusWith<unique_ptr> for ownership transfer"

This reverts commit 3f6f66daac840fe2b2dc26eeeacbf015479567df.
Branch: master
https://github.com/mongodb/mongo/commit/22cf042f9541cf778f71e1bd1a84fcd87012c0b1

Comment by Githook User [ 22/Jun/15 ]

Author:

{u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}

Message: SERVER-16889 query subsystem CanonicalQuery::canonicalize use StatusWith<unique_ptr> for ownership transfer
Branch: master
https://github.com/mongodb/mongo/commit/3f6f66daac840fe2b2dc26eeeacbf015479567df

Comment by J Rassi [ 17/Jun/15 ]

MatchExpressionParser::parse() is not, actually. That method returns a StatusWith<MatchExpression*>, whereas it should instead return StatusWith<std::unique_ptr<MatchExpression>>. It seems fine to me to change the "StatusWithMatchExpression" typedef to refer to the latter instead of the former.

Comment by Qingyang Chen [ 17/Jun/15 ]

LiteParsedQuery::make() and MatchExpressionParser::parse() seems to be taken care of already.

Generated at Thu Feb 08 03:42:37 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.