Multi label and redisearch support #1988
Conversation
* Fix flawed assertion in index deletion logic * Reduce KPI for updates_baseline benchmark * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
* Always report run-time errors as the sole reply * Update test_timeout.py Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #1988 +/- ##
==========================================
- Coverage 92.66% 91.97% -0.70%
==========================================
Files 239 244 +5
Lines 20512 21211 +699
==========================================
+ Hits 19008 19509 +501
- Misses 1504 1702 +198
Continue to review full report at Codecov.
|
jeffreylovitz
left a comment
There was a problem hiding this comment.
I believe there is still a memory leak in dropping full-text indexes:
redis-cli GRAPH.QUERY G "CALL db.idx.fulltext.createNodeIndex({label: 'person', stopwords: ['A', 'B'], language: 'english'}, 'name')"
redis-cli flushdb
redis-cli shutdown
==85154== 220 (206 direct, 14 indirect) bytes in 5 blocks are definitely lost in loss record 1,257 of 1,351
==85154== at 0x4A3A1F9: realloc (vg_replace_malloc.c:836)
==85154== by 0x1584CA: ztryrealloc_usable (zmalloc.c:220)
==85154== by 0x158556: zrealloc (zmalloc.c:250)
==85154== by 0x1EE32C: RM_Realloc (module.c:448)
==85154== by 0x6F43B54: rm_realloc (rmalloc.h:22)
==85154== by 0x6F443C3: __trieMapNode_resizeChildren (triemap.c:31)
==85154== by 0x6F44614: __trieMapNode_AddChild (triemap.c:68)
==85154== by 0x6F44CD1: TrieMapNode_Add (triemap.c:175)
==85154== by 0x6F44C3C: TrieMapNode_Add (triemap.c:167)
==85154== by 0x6F44D35: TrieMap_Add (triemap.c:180)
==85154== by 0x6E35FAC: NewStopWordListCStr (stopwords.c:76)
==85154== by 0x6E35CC2: DefaultStopWordList (stopwords.c:21)
==85154== by 0x6E2D3DE: NewIndexSpec (spec.c:1026)
==85154== by 0x6E1BDFA: RediSearch_CreateIndex (redisearch_api.c:28)
==85154== by 0x5D47DED: Index_Construct (index.c:248)
==85154== by 0x5D8C3E0: Proc_FulltextCreateNodeIdxInvoke (proc_fulltext_create_index.c:147)
==85154== by 0x5D9325A: Proc_Invoke (procedure.c:94)
==85154== by 0x5C3998E: ProcCallConsume (op_procedure_call.c:158)
==85154== by 0x5C5EC97: OpBase_Consume (op.c:45)
==85154== by 0x5C55C5D: ResultsConsume (op_results.c:47)
==85154== by 0x5C5EC97: OpBase_Consume (op.c:45)
==85154== by 0x5C1672B: ExecutionPlan_Execute (execution_plan.c:396)
==85154== by 0x5BEC7F4: _ExecuteQuery (cmd_query.c:182)
==85154== by 0x5DBF9ED: thread_do (thpool.c:380)
==85154== by 0x4DC5608: start_thread (pthread_create.c:477)
And about 5 other similar reports.
As far as I can tell, the serialization logic all seems sound!
| if(index_type == IDX_FULLTEXT) { | ||
| char *lang = RedisModule_LoadStringBuffer(rdb, NULL); | ||
| language = rm_strdup(lang); | ||
| RedisModule_Free(lang); |
There was a problem hiding this comment.
I think the Codecov reports in this file are valid; it would be sensible to have tests that validate the persistency of full-text indexes with customized language and keywords.
|
@jeffreylovitz please let's do live reproduce of the leak |
tests/flow/test_persistency.py
Outdated
| # Save RDB & Load from RDB | ||
| redis_con.execute_command("DEBUG", "RELOAD") |
There was a problem hiding this comment.
# Save RDB & Load from RDB
self.env.dumpAndReload()
tests/flow/test_index_create.py
Outdated
| self.env.assertIn("Index already exists configuration can't be changed", str(e)) | ||
|
|
||
| try: | ||
| # create an index over L1:v4 with stopwords should failed |
There was a problem hiding this comment.
Update comment, we're not setting stopwords here, but language.
tests/flow/test_index_create.py
Outdated
| self.env.assertIn("Stopwords must be array", str(e)) | ||
|
|
||
| try: | ||
| # create an index over L3:v1 with stopwords should failed |
There was a problem hiding this comment.
Update comment, this test checks language.
| self.env.assertIn("Language must be string", str(e)) | ||
|
|
||
|
|
||
| def test03_multi_prop_index_creation(self): |
There was a problem hiding this comment.
Please add:
# try to create an index over person:age and person:name, index shouldn't be created as it already exist
result = redis_graph.query("CREATE INDEX ON :person(name, age)")
self.env.assertEquals(result.indices_created, 0)which reverse the order of attributes.
@AviAvni I looked into this further and don't think it's a real issue. RediSearch instantiates a global default list of stopwords on the creation of the first full-text index, then in the same call replaces that list with the explicitly-provided one. Since the default list can still be used by later indexes, it's not freed - https://github.com/RediSearch/RediSearch/blob/master/src/redisearch_api.c#L51-L53 . The reported leak is just a consequence of us being unable to clean up module globals before exiting. |
…disearch-support Multi label and redisearch support
No description provided.