Resolve race in accessing/updating attribute maps #1165

jeffreylovitz · 2020-06-23T19:41:56Z

This PR adds race condition handling for the attribute mappings, solving any potential issues in our stress test and in general.

It must accommodate a few cases:

Trying to look up an attribute from a reader query, such as the inner _AR_EXP_UpdatePropIdx call in MATCH (x) RETURN x.name.
Adding a new attribute key from a non-locked area, such as in PropertyMap_New while converting an AST's CREATE clause.
Adding a new attribute key from a locked region, such as committing an update in the critical region of OpUpdate.
Adding a new attribute key from a non-Cypher endpoint like bulk insertion.

So far as I can tell, the changes here comprehensively handle all scenarios.

swilly22 · 2020-06-24T06:24:27Z

src/query_ctx.c

@@ -171,6 +179,7 @@ bool QueryCtx_LockForCommit(void) {
 														  strlen(gc->graph_name));
 	char *error;
 	_QueryCtx_ThreadSafeContextLock(ctx);
+	ctx->internal_exec_ctx.locked_for_commit = true;


Why was this line moved?
if locked_for_commit indicates that the graph is locked, (we're OK to modify the graph) then please revert this change.

swilly22 · 2020-06-24T06:26:54Z

src/graph/graphcontext.c

+	// If we are already in a critical region, no further locking should be done in this function.
+	bool in_critical_region = QueryCtx_IsLocked();


Can't we simply try to reacquire the lock, Graph_AcquireReadLock
basically changing the lock to be reentrant.
if we'll do that this function and check can be removed.

swilly22 · 2020-06-24T06:29:28Z

src/graph/graphcontext.c

+	/* If we are not already in a critical region, acquire a read lock.
+	 * Reader threads may acquire a read lock multiple times as long
+	 * as they release the lock the same number of times.
+	 * Writer threads that have not yet entered the critical region will hold no lock at this point,
+	 * while writer threads within the critical region are guaranteed to be the only running thread. */


I believe writer threads acquire a read lock in the same way read threads do, only difference is that writer threads will acquire write lock eventually (when performing the actual modification)

swilly22 · 2020-06-24T06:45:02Z

src/graph/graph.h

+/* Release the held lock without modifying the '_writelocked' flag.  */
+void Graph_ReleaseTemporaryLock(Graph *g);


Our locking logic is complex enough without these additions, can we use the graph RWLock without these additions?
I feel like they make this entire process more complex, if the answer is no, then I would prefer introducing a new mutex guarding the attribute rax and avoid mixing the graph lock with the attribute storage

swilly22 · 2020-06-24T06:58:59Z

src/graph/graphcontext.c

+	// Look up the attribute ID.
+	void *id = raxFind(gc->attributes, (unsigned char *)attribute, strlen(attribute));
+	// Release the lock if acquired.
+	if(!in_critical_region) Graph_ReleaseTemporaryLock(gc->g);


I think it would be cleaner and less confusing / intertwine if we have dedicated RWLock specifically for gc->attributes mixing in the Graph lock make the system more complex, let's keep it simple.

swilly22 · 2020-06-24T06:59:39Z

src/graph/graphcontext.c

@@ -276,9 +287,18 @@ const char *GraphContext_GetAttributeString(const GraphContext *gc, Attribute_ID
 }

 Attribute_ID GraphContext_GetAttributeID(const GraphContext *gc, const char *attribute) {
-	Attribute_ID *id = raxFind(gc->attributes, (unsigned char *)attribute, strlen(attribute));
+	// If we are already in a critical region, no further locking should be done in this function.
+	bool in_critical_region = QueryCtx_IsLocked();


I hope this function isn't called multiple times for a given attribute during the lifetime of a query

swilly22 · 2020-06-24T20:24:40Z

src/graph/graphcontext.c

+		pthread_rwlock_unlock(&gc->_attribute_rwlock);
+		pthread_rwlock_wrlock(&gc->_attribute_rwlock);
+
+		attribute_id = (void *)raxSize(gc->attributes);


This is problematic, consider 2 threads all trying to introduce the same set of attributes: ('a', 'b', 'c')
if thread 1 managed to acquire the lock and insert all 3 attributes, while thread 2 is trying to insert attribute 'a' once 2 gets the lock (trying to introduce 'a') rax size is 3 but the attribute id for 'a' is 1.

swilly22 · 2020-06-24T20:25:59Z

src/graph/graphcontext.c

+		/* rc will be 1 if the insertion was successful and 0 if the rax was not updated,
+		 * which could occur if 2 threads try to retrieve the same attribute simultaneously.
+		 * In this case the second thread will not modify the rax or the string mapping. */
+		int rc = raxTryInsert(gc->attributes,


raxTryInsert will not update if the key exists ?

swilly22 · 2020-06-25T06:01:28Z

src/ast/ast_shared.c

@@ -106,8 +106,7 @@ void PropertyMap_Free(PropertyMap *map) {
 EntityUpdateEvalCtx EntityUpdateEvalCtx_Clone(EntityUpdateEvalCtx ctx) {
 	EntityUpdateEvalCtx clone;


Might be more efficient to use memcpy and AR_EXP_Clone

src/ast/ast_build_op_contexts.c

swilly22 · 2020-06-25T06:07:51Z

src/arithmetic/arithmetic_expression.c

@@ -22,6 +22,9 @@
 #include <ctype.h>
 #include <assert.h>

+// Property keys in variadic expressions will be ATTRIBUTE_UNSET until the first lookup.
+#define ATTRIBUTE_UNSET (USHRT_MAX - 1)


#define ATTRIBUTE_UNSET (ATTRIBUTE_NOTFOUND - 1)

* Update op_node_by_id_seek.c Fix, wrong variable assignment. * Update value.c removed.boolean switch * Add suppression for erroneous leak reports after DEBUG RELOAD (#1094) * Fixed compiler typo (#1097) Paragraph on OSX build should read "CXX" instead of "CPP". * Union bugfixes (#1052) * Improve scoping rules in validating UNION queries * Bugfix in uniquing column containing both nodes and edges * Add flow tests * PR fixes * PR fixes Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Bidirectional expand into (#1047) * wip * Remove unnecessary iterator * WIP * Fix bidirectional ExpandInto, shared edge populating logic * PR fixes * Post-rebase fixes * Fix PR comments * throw runtime-error on missing query parameters (#1100) * removed query parameters annotations (#1101) * do not propagate transpose effect when introducing a transpose operation, maintain expression structure (#1102) * changed RediSearch version to 1.8 (#1103) * changed RediSearch version to 1.8 * added flag for redisearch GC * moved env var setting to memcheck script * Update memcheck.sh * fixed misplaced env var setting * fixed bad command format * flag in circle ci instead of makefile Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * docker file for centos (#1104) * Skip NULL-valued properties in bulk loader (#1108) * Skip NULL-valued properties in bulk loader * Address PR comment, add missing logic in edge procesing * RED-4187: update RS version to 5.6.0 and run on Centos os on CI process (#1112) * Perform one-time transpose of traversed matrices (#1111) * Autoformat * WIP * No-op transpose nodes, working with memory leaks * WIP freeing and simplifying * WIP * Fix false positives on leak checking * Autoformat unit test file * Replace transpose op nodes with operands * Prune unused eval conditions, improve function logic * Fix unit tests * Partially address PR comments * PR fixes continued * PR fixes * Add handling for transpose operations in ApplyTranspose * Fix disconnect sequence in ApplyTranspose on operations Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Omp thread configuration (#1118) * Expose OMP_THREAD_COUNT as a module-level configuration * Update documentation * Improve comments * Refactor logic for module-level configuration * Fix PR comments * Enforce that module args are key-value pairs * PR fixes * PR fixes * redisearch 1.8.1 (#1121) * add doc action * remove internal folders * Add configurations to menu * Update deploy-docs.yaml (#1126) * Update deploy-docs.yaml (#1131) * Null PR for triggering docs (#1132) * Remove deploy-docs (moved to action) (#1140) * update min_redis_pack_version to 5.4.14 (#1142) * Transposed relations (#877) * maintain transposed matrices * Updated unit-tests * Post-rebase fixes * Revert FetchOperands changes * WIP * Unit test fixes * Simplify transpose matrix assignment * Add configuration param * Access config global to check for transposed matrices * Update documentation * Update graph logic to handle the absence of transposed matrices * Fix unit tests * Exit with error on unhandled parameter * Add flow test * Simplify fetch operands logic * Add abstraction layer to check for transposed relations * Partially address PR comments * Update FetchOperands logic * Address PR comments Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * Update index.md (#1143) * Exec plan cache (#1117) * added single threaded LRU cache * changed LRU logic. WIP after design review * fixed PR comments * removed DS_Store * changed pr comments for test priority queue * fixed priority_queue.h and linked_list.h comments. wip * fixed linked_list.c comments. wip * wip * Revert change to queue item sizing * WIP * wip * Remove linear insertion flag, simplify linked list * Refactor cache data structure implementations * Start adding logic for populating cache * added cache size config param * added cache API for graph context * did some clean ups * clone logic * done refactoring. unit tests pass * unit tests pass * wip * wip * test suit pass * test suit pass * fixed some memory leaks * wip * added cache tests * review ready * fixed PR comments * fixed PR comments * fixed PR comments * after rebase * fixed PR comments * fixing memory leaks * trying to avoid race Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * memceck race handling (#1152) * removed time.sleep from tests teardown * added async delete config * fixed config and makefile * moved memcheck to compiler flag * Avoid flushing matrices by maintaining separate transpose edge arrays (#1148) * Avoid flushing matrices by maintaining separate edge arrays in transposes * Properly update transpose matrices on single-edge deletion Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * version bump * fixed parameterized index scan (#1157) * added params support for indexed array lookup (#1159) * removed RedisModule_ReplyWithError from ast validations (#1160) * removed RedisModule_ReplyWithError from ast validations * removed char** reason from functions * refactored query_ctx for varargs Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * added params support to id seek (#1164) * added params support to id seek * reduce to scalar with runtime * fixed PR comments * fixed PR comments * validate graph schema is encoded (#1168) * validate graph schema is encoded * fixed PR comment Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Resolve race in accessing/updating attribute maps (#1165) * Protect critical region of reading/updating GraphContext attributes * Improve lock coverage * Don't heap-alloc attribute ID values * Change to QueryCtx lock check * Bulk insert deadlock fix * Remove FindOrAddAttribute calls from critical region * Revert changes to QueryCtx * Revert changes to Graph * Introduce new rwlock for attribute mapping * Only lookup property ID once * Fix unit tests * Fix possible duplicate entry * Always retrieve attribute value in critical region * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Move index iterator construction to first Consume call (#1169) * Move index iterator construction to first Consume call * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Christoph Zimmermann <40485189+chrisAtRedis@users.noreply.github.com> Co-authored-by: Omri Ben-Gidon <47712555+omrib1@users.noreply.github.com> Co-authored-by: Guy Korland <gkorland@gmail.com> Co-authored-by: Itamar Haber <itamar@redislabs.com> Co-authored-by: swilly22 <roi@redislabs.com>

* Protect critical region of reading/updating GraphContext attributes * Improve lock coverage * Don't heap-alloc attribute ID values * Change to QueryCtx lock check * Bulk insert deadlock fix * Remove FindOrAddAttribute calls from critical region * Revert changes to QueryCtx * Revert changes to Graph * Introduce new rwlock for attribute mapping * Only lookup property ID once * Fix unit tests * Fix possible duplicate entry * Always retrieve attribute value in critical region * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit d66affb)

* Resolve race in accessing/updating attribute maps (#1165) (#1223) * Protect critical region of reading/updating GraphContext attributes * Improve lock coverage * Don't heap-alloc attribute ID values * Change to QueryCtx lock check * Bulk insert deadlock fix * Remove FindOrAddAttribute calls from critical region * Revert changes to QueryCtx * Revert changes to Graph * Introduce new rwlock for attribute mapping * Only lookup property ID once * Fix unit tests * Fix possible duplicate entry * Always retrieve attribute value in critical region * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit d66affb) * Add suppression for erroneous leak reports after DEBUG RELOAD (#1094) (#1210) (cherry picked from commit 386a8e6) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * Update value.c (#1209) removed.boolean switch (cherry picked from commit e5ab233) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * LabelScan with child reads data correctly after reset (#1086) (#1207) (cherry picked from commit cce2ebb) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Error properly on graph accesses of non-graph keys (#1069) (#1206) * Error properly on graph accesses of non-graph keys * Explicitly free QueryCtx on failed delete * Update cmd_delete.c Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit 0cf50df) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * make memcheck rule prints full log for files with leaks (#1072) (#1205) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit 135e0bc) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * ReduceScan and ReduceTraversal respect variable scopes (#1070) (#1204) * ReduceScan and ReduceTraversal respect variable scopes * Fix source lookup * PR fixes Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit 29b75d8) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Better compile-time checking for undefined variables (#1063) (#1203) * Improve AST validations to capture undefined variables * Propagate errors in nested AR_EXP_Evaluate failures (cherry picked from commit e62b823) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * add cloud link and NOT conditions unary validation (#1214) * Guarantee that NOT conditions are unary (#1092) (#1208) (cherry picked from commit b56f98a) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * link to redis cloud Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Pieter Cailliau <pieter.cailliau@gmail.com> * Bidirectional expand into (#1047) * wip * Remove unnecessary iterator * WIP * Fix bidirectional ExpandInto, shared edge populating logic * PR fixes * Post-rebase fixes * Fix PR comments (cherry picked from commit d46c6b6) * do not propagate transpose effect when introducing a transpose operation, maintain expression structure (#1102) (cherry picked from commit d6d6f8b) * added params support to id seek (#1164) (#1218) * added params support to id seek * reduce to scalar with runtime * fixed PR comments * fixed PR comments (cherry picked from commit bc609c3) * Skip NULL-valued properties in bulk loader (#1108) (#1216) * Add suppression for erroneous leak reports after DEBUG RELOAD (#1094) (#1210) (cherry picked from commit 386a8e6) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * Update value.c (#1209) removed.boolean switch (cherry picked from commit e5ab233) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * LabelScan with child reads data correctly after reset (#1086) (#1207) (cherry picked from commit cce2ebb) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Error properly on graph accesses of non-graph keys (#1069) (#1206) * Error properly on graph accesses of non-graph keys * Explicitly free QueryCtx on failed delete * Update cmd_delete.c Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit 0cf50df) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * make memcheck rule prints full log for files with leaks (#1072) (#1205) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit 135e0bc) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * ReduceScan and ReduceTraversal respect variable scopes (#1070) (#1204) * ReduceScan and ReduceTraversal respect variable scopes * Fix source lookup * PR fixes Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit 29b75d8) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Better compile-time checking for undefined variables (#1063) (#1203) * Improve AST validations to capture undefined variables * Propagate errors in nested AR_EXP_Evaluate failures (cherry picked from commit e62b823) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * add cloud link and NOT conditions unary validation (#1214) * Guarantee that NOT conditions are unary (#1092) (#1208) (cherry picked from commit b56f98a) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * link to redis cloud Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Pieter Cailliau <pieter.cailliau@gmail.com> * Bidirectional expand into (#1047) * wip * Remove unnecessary iterator * WIP * Fix bidirectional ExpandInto, shared edge populating logic * PR fixes * Post-rebase fixes * Fix PR comments (cherry picked from commit d46c6b6) * Skip NULL-valued properties in bulk loader (#1108) * Skip NULL-valued properties in bulk loader * Address PR comment, add missing logic in edge procesing (cherry picked from commit b62e802) * do not propagate transpose effect when introducing a transpose operation, maintain expression structure (#1102) (cherry picked from commit d6d6f8b) * added params support to id seek (#1164) (#1218) * added params support to id seek * reduce to scalar with runtime * fixed PR comments * fixed PR comments (cherry picked from commit bc609c3) * Move index iterator construction to first Consume call (#1169) (#1219) * Move index iterator construction to first Consume call * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit 7853153) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Pieter Cailliau <pieter.cailliau@gmail.com> * add GTM (#1237) (cherry picked from commit 8cc66d8) * fixed graph context creation * Update index on change (#1225) * WIP * Fix compile errors * Only update indexes when necessary * refined update entity update eval * WIP * WIP * Compilation fixes, edge updates * Add freeing logic * Update comments * some minor changes to op_update * Address PR comments * Address PR comments * always get updated node label id Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: DvirDukhan <dvir@redislabs.com> (cherry picked from commit e712193) * Update index on change (#1225) * WIP * Fix compile errors * Only update indexes when necessary * refined update entity update eval * WIP * WIP * Compilation fixes, edge updates * Add freeing logic * Update comments * some minor changes to op_update * Address PR comments * Address PR comments * always get updated node label id Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: DvirDukhan <dvir@redislabs.com> (cherry picked from commit e712193) * memceck race handling (#1152) * removed time.sleep from tests teardown * added async delete config * fixed config and makefile * moved memcheck to compiler flag (cherry picked from commit b7d0a38) Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Pieter Cailliau <pieter.cailliau@gmail.com> Co-authored-by: Guy Korland <gkorland@gmail.com>

* Update op_node_by_id_seek.c Fix, wrong variable assignment. * Update value.c removed.boolean switch * Add suppression for erroneous leak reports after DEBUG RELOAD (#1094) * Fixed compiler typo (#1097) Paragraph on OSX build should read "CXX" instead of "CPP". * Union bugfixes (#1052) * Improve scoping rules in validating UNION queries * Bugfix in uniquing column containing both nodes and edges * Add flow tests * PR fixes * PR fixes Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Bidirectional expand into (#1047) * wip * Remove unnecessary iterator * WIP * Fix bidirectional ExpandInto, shared edge populating logic * PR fixes * Post-rebase fixes * Fix PR comments * throw runtime-error on missing query parameters (#1100) * removed query parameters annotations (#1101) * do not propagate transpose effect when introducing a transpose operation, maintain expression structure (#1102) * changed RediSearch version to 1.8 (#1103) * changed RediSearch version to 1.8 * added flag for redisearch GC * moved env var setting to memcheck script * Update memcheck.sh * fixed misplaced env var setting * fixed bad command format * flag in circle ci instead of makefile Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * docker file for centos (#1104) * Skip NULL-valued properties in bulk loader (#1108) * Skip NULL-valued properties in bulk loader * Address PR comment, add missing logic in edge procesing * RED-4187: update RS version to 5.6.0 and run on Centos os on CI process (#1112) * Perform one-time transpose of traversed matrices (#1111) * Autoformat * WIP * No-op transpose nodes, working with memory leaks * WIP freeing and simplifying * WIP * Fix false positives on leak checking * Autoformat unit test file * Replace transpose op nodes with operands * Prune unused eval conditions, improve function logic * Fix unit tests * Partially address PR comments * PR fixes continued * PR fixes * Add handling for transpose operations in ApplyTranspose * Fix disconnect sequence in ApplyTranspose on operations Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Omp thread configuration (#1118) * Expose OMP_THREAD_COUNT as a module-level configuration * Update documentation * Improve comments * Refactor logic for module-level configuration * Fix PR comments * Enforce that module args are key-value pairs * PR fixes * PR fixes * redisearch 1.8.1 (#1121) * add doc action * remove internal folders * Add configurations to menu * Update deploy-docs.yaml (#1126) * Update deploy-docs.yaml (#1131) * Null PR for triggering docs (#1132) * Remove deploy-docs (moved to action) (#1140) * update min_redis_pack_version to 5.4.14 (#1142) * Transposed relations (#877) * maintain transposed matrices * Updated unit-tests * Post-rebase fixes * Revert FetchOperands changes * WIP * Unit test fixes * Simplify transpose matrix assignment * Add configuration param * Access config global to check for transposed matrices * Update documentation * Update graph logic to handle the absence of transposed matrices * Fix unit tests * Exit with error on unhandled parameter * Add flow test * Simplify fetch operands logic * Add abstraction layer to check for transposed relations * Partially address PR comments * Update FetchOperands logic * Address PR comments Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * Update index.md (#1143) * Exec plan cache (#1117) * added single threaded LRU cache * changed LRU logic. WIP after design review * fixed PR comments * removed DS_Store * changed pr comments for test priority queue * fixed priority_queue.h and linked_list.h comments. wip * fixed linked_list.c comments. wip * wip * Revert change to queue item sizing * WIP * wip * Remove linear insertion flag, simplify linked list * Refactor cache data structure implementations * Start adding logic for populating cache * added cache size config param * added cache API for graph context * did some clean ups * clone logic * done refactoring. unit tests pass * unit tests pass * wip * wip * test suit pass * test suit pass * fixed some memory leaks * wip * added cache tests * review ready * fixed PR comments * fixed PR comments * fixed PR comments * after rebase * fixed PR comments * fixing memory leaks * trying to avoid race Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * memceck race handling (#1152) * removed time.sleep from tests teardown * added async delete config * fixed config and makefile * moved memcheck to compiler flag * Avoid flushing matrices by maintaining separate transpose edge arrays (#1148) * Avoid flushing matrices by maintaining separate edge arrays in transposes * Properly update transpose matrices on single-edge deletion Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * fixed parameterized index scan (#1157) * added params support for indexed array lookup (#1159) * removed RedisModule_ReplyWithError from ast validations (#1160) * removed RedisModule_ReplyWithError from ast validations * removed char** reason from functions * refactored query_ctx for varargs Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * added params support to id seek (#1164) * added params support to id seek * reduce to scalar with runtime * fixed PR comments * fixed PR comments * validate graph schema is encoded (#1168) * validate graph schema is encoded * fixed PR comment Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Resolve race in accessing/updating attribute maps (#1165) * Protect critical region of reading/updating GraphContext attributes * Improve lock coverage * Don't heap-alloc attribute ID values * Change to QueryCtx lock check * Bulk insert deadlock fix * Remove FindOrAddAttribute calls from critical region * Revert changes to QueryCtx * Revert changes to Graph * Introduce new rwlock for attribute mapping * Only lookup property ID once * Fix unit tests * Fix possible duplicate entry * Always retrieve attribute value in critical region * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Move index iterator construction to first Consume call (#1169) * Move index iterator construction to first Consume call * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * fix graph creation example (#1171) * fix graph creation example * fixed errors in example * single flow (#1177) * added explain and profile options as invalid options in redisgraph (#1184) * added explain and profile options as invalid options in redisgraph query string * fixed PR comments * moved tests * Update Dockerfile (#1188) * Adding redis cloud pro to quick start Please hold back merging this PR. Thanks * fix typo in anker * enable search GC (#1194) * aggregated slowlog should maintain original timestamps (#1199) * correct link to redis cloud * Aumation auth moved to be token based (#1192) * Aumation auth moved to be token based * Update config.yml * Update config.yml * relay on search replace add functionality to delete existing documents (#1226) * line length 80 (#1227) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Emit compile time error on creation of undirected edges (#1212) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * redisearch 1.8.2 (#1229) * redisearch 1.8.2 * linked search cleanup function * add GTM (#1239) * Update index on change (#1225) * WIP * Fix compile errors * Only update indexes when necessary * refined update entity update eval * WIP * WIP * Compilation fixes, edge updates * Add freeing logic * Update comments * some minor changes to op_update * Address PR comments * Address PR comments * always get updated node label id Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: DvirDukhan <dvir@redislabs.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Christoph Zimmermann <40485189+chrisAtRedis@users.noreply.github.com> Co-authored-by: Omri Ben-Gidon <47712555+omrib1@users.noreply.github.com> Co-authored-by: Guy Korland <gkorland@gmail.com> Co-authored-by: Itamar Haber <itamar@redislabs.com> Co-authored-by: Martin Rauscher <hades32@gmail.com> Co-authored-by: Pieter Cailliau <pieter.cailliau@gmail.com>

* Protect critical region of reading/updating GraphContext attributes * Improve lock coverage * Don't heap-alloc attribute ID values * Change to QueryCtx lock check * Bulk insert deadlock fix * Remove FindOrAddAttribute calls from critical region * Revert changes to QueryCtx * Revert changes to Graph * Introduce new rwlock for attribute mapping * Only lookup property ID once * Fix unit tests * Fix possible duplicate entry * Always retrieve attribute value in critical region * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

jeffreylovitz added the bug label Jun 23, 2020

jeffreylovitz added this to the RedisGraph 2.2 milestone Jun 23, 2020

jeffreylovitz requested review from swilly22 and DvirDukhan June 23, 2020 19:41

jeffreylovitz self-assigned this Jun 23, 2020

jeffreylovitz linked an issue Jun 23, 2020 that may be closed by this pull request

circle CI build failed #1153

Closed

swilly22 requested changes Jun 24, 2020

View reviewed changes

jeffreylovitz added 10 commits June 24, 2020 12:51

Protect critical region of reading/updating GraphContext attributes

6ae7d2b

Improve lock coverage

a335aad

Don't heap-alloc attribute ID values

1adc5d8

Change to QueryCtx lock check

18c0a81

Bulk insert deadlock fix

fee3b96

Remove FindOrAddAttribute calls from critical region

e0cc612

Revert changes to QueryCtx

e404a5b

Revert changes to Graph

8be01b2

Introduce new rwlock for attribute mapping

fb6dae6

Only lookup property ID once

c1c8814

jeffreylovitz force-pushed the attribute-map-race branch from ec1192a to c1c8814 Compare June 24, 2020 16:59

jeffreylovitz added 2 commits June 24, 2020 13:19

Fix unit tests

b3f6012

Fix possible duplicate entry

9273f6b

swilly22 requested changes Jun 24, 2020

View reviewed changes

Always retrieve attribute value in critical region

b0ea7c1

swilly22 requested changes Jun 25, 2020

View reviewed changes

jeffreylovitz and others added 3 commits June 25, 2020 08:18

Address PR comments

90d2449

Merge branch 'master' into attribute-map-race

ffa4fa0

Address PR comments

28e428d

swilly22 approved these changes Jun 25, 2020

View reviewed changes

swilly22 merged commit d66affb into master Jun 25, 2020

swilly22 deleted the attribute-map-race branch June 25, 2020 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve race in accessing/updating attribute maps #1165

Resolve race in accessing/updating attribute maps #1165

jeffreylovitz commented Jun 23, 2020

swilly22 Jun 24, 2020

swilly22 Jun 24, 2020

swilly22 Jun 24, 2020

swilly22 Jun 24, 2020

swilly22 Jun 24, 2020

swilly22 Jun 24, 2020

swilly22 Jun 24, 2020

swilly22 Jun 24, 2020

swilly22 Jun 25, 2020

swilly22 Jun 25, 2020 •

edited

Loading

		// If we are already in a critical region, no further locking should be done in this function.
		bool in_critical_region = QueryCtx_IsLocked();

		/* Release the held lock without modifying the '_writelocked' flag. */
		void Graph_ReleaseTemporaryLock(Graph *g);

		@@ -106,8 +106,7 @@ void PropertyMap_Free(PropertyMap *map) {
		EntityUpdateEvalCtx EntityUpdateEvalCtx_Clone(EntityUpdateEvalCtx ctx) {
		EntityUpdateEvalCtx clone;

Resolve race in accessing/updating attribute maps #1165

Resolve race in accessing/updating attribute maps #1165

Conversation

jeffreylovitz commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swilly22 Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

swilly22 Jun 25, 2020 •

edited

Loading