-
Notifications
You must be signed in to change notification settings - Fork 406
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
Add EdgeDB query performance statistics #7814
Conversation
But we don't have to, do we? If we don't recognize a query as being a compiler product we can avoid modifying its state. |
e49e222
to
eee60ba
Compare
d59be85
to
fb8ce27
Compare
Also: * Track normalized EdgeQL instead of original * Track each command in scripts * Skip info header if backend doesn't support stats * Add back debug.edgeql_text_in_sql * Exclude info header from the SQL hash
static uint64 | ||
edbss_extract_stmt_info(char **query_str) { | ||
if (strncmp(*query_str, EDB_STMT_MAGIC_PREFIX, strlen(EDB_STMT_MAGIC_PREFIX)) == 0) { | ||
EdbStmtInfoSemState state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What version of C will this be compiled with? Because you could just do this in C99:
EdbStmtInfoSemState state; | |
EdbStmtInfoSemState state = {0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, mostly assuming C99 in #7869 now
setting_filter=lambda v: v.name in spec | ||
and spec[v.name].affects_compilation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make sure nothing secret
goes in also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only compilation setting that may include free text is force_database_error
, which shouldn't have secrets. I can also add a double-safe to filter out extension configs in #7869.
sql_info: Dict[str, Any] = {} | ||
if ( | ||
not ctx.bootstrap_mode | ||
and ctx.backend_runtime_params.has_stat_statements | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want whether we collect this to also depend on a config flag? This seems like a lot of extra stuff to store unconditionally.
(Curious about @elprans's opinion on this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered in the meeting: we do need a config to turn the tracking on/off.
Follow-up question: do we want to make this config affects_compilation
(thus controlling the compiled SQL with or without the magic JSON-comment prefix, but impacting query cache), or a runtime session config that the extension can read and behave (but we end up with prefixed SQLs all the time)?
Refs #7725, this is
a quick'n'dirty first attemptthe first implementation with a standalone extension that manipulates the PG query source text (and optionally the queryID to map to the cache key). The upper side is that the extension is kinda clean without patching PG codebase (easier to port),the down side is that we lose the raw SQL stats data for all.Sample results: