Fix memory leak on allocated keys in OpAggregate #1650

jeffreylovitz · 2021-04-02T19:40:27Z

Resolves a memory leak when the key to an aggregate expression is heap-allocated, as in STR1 and STR2 in:

UNWIND ['str1', 'str1', 'str2', 'str1'] AS key UNWIND [1, 2, 3] as agg RETURN toUpper(key) AS key, collect(DISTINCT agg)

swilly22

@jeffreylovitz please see request for comments

swilly22 · 2021-04-04T11:25:09Z

tests/flow/test_map.py

+    # Validate that map projections with invalid identifiers error gracefully.
+    def test07_map_invalid_identifier(self):
+        try:
+            query = """RETURN 5 {v: 'b'}"""


so strange this is valid cypher:

./cypher-lint -1 -a RETURN 5 {v: 'b'}; @0 0..18 statement body=@1 @1 0..18 > query clauses=[@2] @2 0..17 > > RETURN projections=[@3] @3 7..17 > > > projection expression=@4, alias=@9 @4 7..17 > > > > map projection @5{@6} @5 7..8 > > > > > integer 5 @6 10..16 > > > > > literal projection @7: @8 @7 10..11 > > > > > > prop name `v` @8 13..16 > > > > > > string "b" @9 7..17 > > > > identifier `5 {v: 'b'}`

swilly22 · 2021-04-04T11:30:33Z

src/serializers/decoders/current/v9/decode_graph_entities.c

+		SIValue elem = _RdbLoadSIValue(rdb);
+		SIArray_Append(&list, elem);
+		SIValue_Free(elem);


Right, array need to protect its elements by calling SI_CloneValue internally, it's easy to forget that, which will lead to leaks.

I feel like we should try switching to a reference counter for SIValues soon; it will reduce the number of redundant allocs greatly.

Agree, we need to consider this option

swilly22 · 2021-04-04T11:52:04Z

src/execution_plan/ops/op_aggregate.c

+	for(uint i = 0; i < op->key_count; i++) {
+		SIValue_Free(op->group_keys[i]);
+	}


Add a short comment explaining why this logic is necessary

swilly22 · 2021-04-04T11:54:06Z

src/arithmetic/arithmetic_expression_construct.c

@@ -355,7 +355,10 @@ static AR_ExpNode *_AR_ExpFromMapProjection(const cypher_astnode_t *expr) {

 	cypher_astnode_type_t t;
 	const cypher_astnode_t *identifier = cypher_ast_map_projection_get_expression(expr);
-	ASSERT(cypher_astnode_type(identifier) == CYPHER_AST_IDENTIFIER);
+	if(cypher_astnode_type(identifier) != CYPHER_AST_IDENTIFIER) {


Can you please add a short comment explaining this?

swilly22

Perfect, thanks!

* Fix memory leak on allocated keys in OpAggregate * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <[email protected]> (cherry picked from commit 096373d)

* Shortest path func (#1627) * WIP * add BFS to destination algorithm * WIP * Add shortestPath function call * Revert changes around OpShortestPath * Add test cases * One-time creation of matrices for shortestPath * formatting * Address PR comments * Add documentation * Update commands.md * search for path object only under specific clauses * Update LAGraph_bfs_pushpull.h * Address PR comments * Don't cache schema IDs in shortestPath Co-authored-by: swilly22 <[email protected]> Co-authored-by: Roi Lipman <[email protected]> (cherry picked from commit 98aa629) * Fix: Updated benchmark readme with newest specifications ( and simplification also ). Updated benchmark automation wrapper version. (#1633) (cherry picked from commit 03543c0) * add Julia client (#1636) (cherry picked from commit 484faa7) * Julia client (#1637) * add Julia client * update client list (cherry picked from commit 5388836) * Fix label retrieval on cached unknown value (#1644) (cherry picked from commit 4fa2c1b) * GRAPH.LIST lists all graph keys in keyspace (no support for cluster env) (#1649) * list graphs in keyspace * test graph.list * add graph.list to commands doc (cherry picked from commit bc33380) * add sqrt function (#1653) (cherry picked from commit 2de9fcf) * Sqrt (#1655) * add sqrt function * switch to std:isnan (cherry picked from commit 1f056a2) * Fix memory leak on allocated keys in OpAggregate (#1650) * Fix memory leak on allocated keys in OpAggregate * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <[email protected]> (cherry picked from commit 096373d) * drain thread pools before shutting down (#1656) * drain thread pools before shutting down * pause worker threads on shutdown * skip thread pools clean up on server shutdown (cherry picked from commit 1782e83) * Support filtering on variable-length edges in traversals (#1657) * Support filtering on variable-length edges in traversals * concatenate multiple variable edge filters into a single filter tree * Address PR comments * add path object to clone record Co-authored-by: swilly22 <[email protected]> Co-authored-by: Roi Lipman <[email protected]> (cherry picked from commit 9204660) * The default warnings.warn prints itself, this function should override the default behavior (#1662) Co-authored-by: Ofir Moskovich <[email protected]> (cherry picked from commit d9a50ed) * Limit threadpool queue size (#1664) * limit number of pending queries * init config as part of testing * free thread safe ctx only for blocked clients * test max pending query limit * Add documentation * Address PR comments * Attempt to fix test on CI * Resolve memory leak * wrong thread count Co-authored-by: Jeffrey Lovitz <[email protected]> (cherry picked from commit 2b88d39) * Fix CI benchmarks on version tag (#1665) (cherry picked from commit 93d87ac) * Add benchmark to validate variable-length edge filters (#1667) (cherry picked from commit e8b5e94) * Apply path filters after all other filters (#1668) * Apply path filters after all other filters * switch to a single query construction * Use dedicated relocate function * Update execution_plan_construct.c Co-authored-by: swilly22 <[email protected]> Co-authored-by: Roi Lipman <[email protected]> (cherry picked from commit e084bb6) * Runtime index utilisation (#1641) * WIP migrate search query composition from utilize_indices to a utility file * [WIP] runtime index query construction * resolve compile-time unknowns * entity functions are reduceable if the entity is known * test index runtime query construction * normalize filter, make sure filtered entity is on the left hand side * free index scan filter tree * return none converted filters * WIP * WIP * index none indexed field names * pull from index until unresolved filter pass or iterator deplete * test index scan over none indexable values * remove include of deleted file * rebase conflicts resolved * address PR comments * change index separator (cherry picked from commit 247d747) * disallow aggregations in maps (#1671) * disalow aggregations in maps * Add documentation, expand on error message * Skip invalidated TCK tests Co-authored-by: Jeffrey Lovitz <[email protected]> (cherry picked from commit e86b5e3) Co-authored-by: filipe oliveira <[email protected]> Co-authored-by: Roi Lipman <[email protected]> Co-authored-by: OfirMos <[email protected]>

* Fix memory leak on allocated keys in OpAggregate * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <[email protected]>

Fix memory leak on allocated keys in OpAggregate

e664d02

jeffreylovitz added the bug label Apr 2, 2021

jeffreylovitz requested a review from swilly22 April 2, 2021 19:40

swilly22 previously approved these changes Apr 4, 2021

View reviewed changes

Address PR comments

04829af

jeffreylovitz dismissed swilly22’s stale review via 04829af April 5, 2021 12:47

swilly22 and others added 2 commits April 5, 2021 15:55

Merge branch 'master' into aggregate-key-leak

b189b10

Address PR comments

8b85a96

swilly22 approved these changes Apr 6, 2021

View reviewed changes

Merge branch 'master' into aggregate-key-leak

86cda78

swilly22 approved these changes Apr 6, 2021

View reviewed changes

swilly22 merged commit 096373d into master Apr 6, 2021

swilly22 deleted the aggregate-key-leak branch April 6, 2021 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix memory leak on allocated keys in OpAggregate #1650

Fix memory leak on allocated keys in OpAggregate #1650

Uh oh!

jeffreylovitz commented Apr 2, 2021

Uh oh!

swilly22 left a comment

Uh oh!

swilly22 Apr 4, 2021

Uh oh!

swilly22 Apr 4, 2021

Uh oh!

jeffreylovitz Apr 5, 2021

Uh oh!

swilly22 Apr 6, 2021

Uh oh!

swilly22 Apr 4, 2021

Uh oh!

swilly22 Apr 4, 2021

Uh oh!

swilly22 left a comment

Uh oh!

Uh oh!

Fix memory leak on allocated keys in OpAggregate #1650

Fix memory leak on allocated keys in OpAggregate #1650

Uh oh!

Conversation

jeffreylovitz commented Apr 2, 2021

Uh oh!

swilly22 left a comment

Choose a reason for hiding this comment

Uh oh!

swilly22 Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

swilly22 Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

jeffreylovitz Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

swilly22 Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

swilly22 Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

swilly22 Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

swilly22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!