Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support multi-exec #352

Merged
merged 2 commits into from
Feb 11, 2019
Merged

support multi-exec #352

merged 2 commits into from
Feb 11, 2019

Conversation

swilly22
Copy link
Collaborator

@swilly22 swilly22 commented Feb 10, 2019

#333

MULTI
GRAPH.QUERY G QUERY
GRAPH.QUERY G QUERY
GRAPH.QUERY G QUERY
EXEC

@swilly22 swilly22 added this to the 1.2.0 milestone Feb 10, 2019
@swilly22 swilly22 self-assigned this Feb 10, 2019
@swilly22 swilly22 added this to In progress in RedisGraph 2.0 via automation Feb 10, 2019
Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one comment.

GraphContext *gc = GraphContext_Retrieve(ctx, qctx->graphName);
RedisModule_ThreadSafeContextUnlock(ctx);

CommandCtx_ThreadSafeContextUnlock(qctx);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theadeSafeCtx should be released only after the 'if(!gc)' block to prevent edge case of two writers that create a new graph simultaneously and one of them failed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

MeirShpilraien
MeirShpilraien previously approved these changes Feb 10, 2019
context->argc = argc;

// Make a copy of graph name.
if(graphName) context->graphName = rm_strdup(RedisModule_StringPtrLen(graphName, NULL));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set context->graphName to NULL if not provided.

}

if(qctx->ast) AST_Free(qctx->ast);
rm_free(qctx->graphName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Safer to have an if condition on this free.

// Either we already have a context or block client is set.
if(qctx->ctx) return qctx->ctx;

assert(!qctx->ctx && qctx->bc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left-hand condition unnecessary.


// Replicate only if query has potential to modify key space.
bool readonly = AST_ReadOnly(ast);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid read if we attached an AST to the QueryContext, since ast is freed by now.

Copy link
Contributor

@jeffreylovitz jeffreylovitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution! A few minor notes, but everything looks good to me.

@swilly22 swilly22 merged commit 3515b0e into master Feb 11, 2019
RedisGraph 2.0 automation moved this from In progress to Done Feb 11, 2019
@swilly22 swilly22 deleted the multi-exec branch February 11, 2019 09:03
pnxguide pushed a commit to CMU-SPEED/RedisGraph that referenced this pull request Mar 22, 2023
* support multi-exec

* deal with multiple concurrent writers, when graph does not exists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants