Skip to content

Share Query Cache feature #250

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

Conversation

SergeiPavlov
Copy link
Contributor

Setting DomainConfiguration.ShareQueryCacheOverNodes = true enables to share compiled queries between nodes

@alex-kulakov
Copy link
Contributor

No tests for this? This is really important. In case if cached query reached wrong node it would be a huge problem, we need to be 100% sure that this is not happening.

@SergeiPavlov
Copy link
Contributor Author

No tests for this? This is really important. In case if cached query reached wrong node it would be a huge problem, we need to be 100% sure that this is not happening.

I have little knowledge how to write multi-node DO test. But we are using this feature many months in production.

@alex-kulakov
Copy link
Contributor

alex-kulakov commented Jul 14, 2022

I have little knowledge how to write multi-node DO test.

You can ask for help. I'll guide and help you, not a big deal.

But we are using this feature many months in production.

No offence but it'd rather choose proper test than "it works in our environment". I don't know your environment, I don't know how you use it. As I said I'll help you with tests.

@SergeiPavlov
Copy link
Contributor Author

No offence but it'd rather choose proper test than "it works in our environment". I don't know your environment, I don't know how you use it. As I said I'll help you with tests.

Thank you. I hope I will have time to make tests. But now it is not high priority task

@alex-kulakov
Copy link
Contributor

alex-kulakov commented Aug 12, 2022

I don't like how upper-level types like TypeInfo and StorageNode got into SQL level. SQLDOM doesn't have to know about domains, sessions, typeinfos, storage nodes, etc. It is basically stand-alone, having just an SqlDriver, it should be allowed to extract schemas and catalogs, create and compile any SQL query supported by SQLDOM.

There is got to be a way to create an equivalent of the query created by DO by using SQLDOM and certain SqlDriver only, in this case it is impossible.

Comment on lines +1519 to +1521
case TypeInfo typeInfo:
output.AppendPlaceholderWithId(typeInfo);
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

For example this is completely inappropriate.

@@ -37,13 +38,13 @@ public class SqlCompiler : SqlDriverBound,
/// <param name="unit">Instance to compile.</param>
/// <param name="compilerConfiguration">Configuration for compiler.</param>
/// <returns></returns>
public SqlCompilationResult Compile(ISqlCompileUnit unit, SqlCompilerConfiguration compilerConfiguration)
public SqlCompilationResult Compile(ISqlCompileUnit unit, SqlCompilerConfiguration compilerConfiguration, TypeIdRegistry typeIdRegistry = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also not ok, you have a configuration that wraps additional info for compilation. But no upper-level types should be used there.

@alex-kulakov
Copy link
Contributor

I'm also thinking of having caches within nodes in cases when DomainConfiguration.ShareQueryCacheOverNodes = false but I'm weighting the outcome of this. Now number of items in query cache is multiplied by number of nodes, it should affect performance on thousands of nodes and thousands of queries (btw, how big is your cache size?).

Logically the idea of separated caches fits the concept attachable/detachable node - once node is disposed the resources it occupied should be released too.

@SergeiPavlov
Copy link
Contributor Author

There is got to be a way to create an equivalent of the query created by DO by using SQLDOM and certain SqlDriver only, in this case it is impossible.

I agree this is some kind of hack. But it just works, that is why I decided to use it.

@alex-kulakov
Copy link
Contributor

Can you list the parts of query you replace with placeholders or something similar to make query to be cacheable?

@SergeiPavlov
Copy link
Contributor Author

Can you list the parts of query you replace with placeholders or something similar to make query to be cacheable?

  • schema
  • TypeId

@alex-kulakov
Copy link
Contributor

Long time ago I did TypeId as parameter of query, but Alexander Ustinov said that there is something wrong with my implementation if applied to your case so i didn't merge it. Now I have a second thought that maybe it will somehow fit to this concept, ShareQueryCacheOverNodes requires basically two features combined - TypeIdAsParameter and ShareSchemaOverNodes, doesn't it?

@SergeiPavlov
Copy link
Contributor Author

requires basically two features combined - TypeIdAsParameter and ShareSchemaOverNodes, doesn't it?

Looks like, yes
If you are talkimg about your year-old implementation of sharedCache-like feature, I considered to use, but was not suitable for our case (does not remember why already)

@alex-kulakov
Copy link
Contributor

I have improved TypeId As Parameter PR (#147), It is based on QueryParameterBinding now which is much better.

@alex-kulakov
Copy link
Contributor

Additionally, I was inspired by your idea and changed schema actualization, here's the PR (#289). I think we could combine these two. PRs (#147 and #289) and just introduce condition like

var domainConfig = domain.Configuration;
this.queryKey = domainConfig.ShareSchemaOverNodes && domainConfig.PreferTypeIdsAsQueryParameters
  ? queryKey
  : new Pair<object, string>(queryKey, session.StorageNodeId);

in this PR.

@SergeiPavlov
Copy link
Contributor Author

just introduce condition like

done

@alex-kulakov
Copy link
Contributor

alex-kulakov commented Nov 29, 2022

Take a look at the implementation in master-shared-cache. It has tests for the feature as well. I need it to be tested on various database configs and then it will be ready for pull request

@alex-kulakov
Copy link
Contributor

The PR is ready - #290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants