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

Add query performance statistics #7869

Merged
merged 32 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
4b8d7b4
Fork the official pg_stat_statements extension from d5ca15ee54bf7faf0…
fantix Oct 15, 2024
6f36dca
Rename folder pg_stat_statements -> edb_stat_statements
fantix Oct 15, 2024
5eab017
Drop meson build for easier maintanence
fantix Oct 15, 2024
d363d78
Squash extension versions into 1.0
fantix Oct 15, 2024
75004d5
Rename the extension to edb_stat_statements
fantix Oct 15, 2024
21c11aa
Add editorconfig for PG extension
fantix Oct 16, 2024
3e459e2
Rename functions, views, gucs and namespaces to edb_stat_statements
fantix Oct 16, 2024
0a79cd4
Support Postgres 16, 17 and 18
fantix Oct 16, 2024
a98b33e
Build the extension in dev environment
fantix Sep 27, 2024
e3b512f
Honor EDGEDB_DEBUG=1 in building dev Postgres
fantix Oct 16, 2024
2929831
Add test to run extension installcheck
fantix Oct 16, 2024
284cbec
Add new backend capabilities for the extension
fantix Oct 4, 2024
158cc6b
Turn off stats without superuser access
fantix Oct 4, 2024
d76eb07
Main: add sys::QueryStats through edb_stat_statements
fantix Oct 16, 2024
4c5ec5f
Only show stats of the current tenant
fantix Oct 5, 2024
f1153ed
Implement the reset function
fantix Oct 5, 2024
ed36827
Track SQL queries in stats
fantix Oct 5, 2024
8e74a90
Merge remote-tracking branch 'origin/master' into query-stats-4
fantix Oct 22, 2024
44154bb
CRF: add comments and manually manage lifetime
fantix Oct 22, 2024
ac3b25e
Merge remote-tracking branch 'origin/master' into query-stats-4
fantix Oct 23, 2024
87c2c96
Merge remote-tracking branch 'origin/master' into query-stats-4
fantix Oct 24, 2024
cd5d408
Merge remote-tracking branch 'origin/master' into query-stats-4
fantix Nov 12, 2024
eb453cf
CRF: add annotations and fix test
fantix Nov 12, 2024
575517e
Merge remote-tracking branch 'origin/master' into query-stats-4
fantix Nov 15, 2024
1bc09fe
Fix merge
fantix Nov 17, 2024
f26c9c5
Allow multiple info lines in SQL
fantix Nov 17, 2024
ccaf5ac
Merge query_id and cache_key into a single `id`
fantix Nov 18, 2024
00f9cd5
Stop exposing query_id field, use id instead
fantix Nov 18, 2024
c7e861b
Merge remote-tracking branch 'origin/master' into query-stats-4
fantix Nov 19, 2024
9e1b40b
Add compilation settings as extras jsonb field
fantix Oct 7, 2024
ac41168
Don't add info JSON in DDL function body
fantix Nov 20, 2024
c5bec55
Add comments
fantix Nov 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions edb_stat_statements/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ ifeq ($(shell test $(PG_MAJOR) -ge 18 && echo true), true)
REGRESS += extended
endif

all:

expected/dml.out:
if [ $(PG_MAJOR) -ge 18 ]; then \
cp expected/dml.out.18 expected/dml.out; \
Expand Down
89 changes: 54 additions & 35 deletions edb_stat_statements/edb_stat_statements.c
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@

PG_MODULE_MAGIC;

#define EDB_STMT_MAGIC_PREFIX "-- {\"query\""
#define EDB_STMT_MAGIC_PREFIX "-- {"

/* Location of permanent stats file (valid when database is shut down) */
#define PGSS_DUMP_FILE PGSTAT_STAT_PERMANENT_DIRECTORY "/edb_stat_statements.stat"
Expand Down Expand Up @@ -371,6 +371,8 @@ PG_FUNCTION_INFO_V1(edb_stat_statements_reset);
PG_FUNCTION_INFO_V1(edb_stat_statements);
PG_FUNCTION_INFO_V1(edb_stat_statements_info);

const char *
edbss_extract_info_line(const char *s, int* len);
EdbStmtInfo *
edbss_extract_stmt_info(const char* query_str, int query_len);
static inline void
Expand Down Expand Up @@ -559,46 +561,39 @@ _PG_init(void)
ProcessUtility_hook = pgss_ProcessUtility;
}

const char *
edbss_extract_info_line(const char *s, int *len) {
int prefix_len = strlen(EDB_STMT_MAGIC_PREFIX);
if (*len > prefix_len && strncmp(s, EDB_STMT_MAGIC_PREFIX, prefix_len) == 0) {
const char *rv = s + 3; // skip "-- "
int remaining_len = *len - prefix_len;
int rv_len = 0;
while (rv_len < remaining_len && rv[rv_len] != '\n')
rv_len++;
if (rv_len > 0) {
*len = rv_len;
return rv;
}
}
return NULL;
}

/*
* Extract EdgeDB query info from the JSON in the leading comment.
* If success, returns a palloc-ed EdbStmtInfo which must be freed
* after usage with edbss_free_stmt_info().
*/
EdbStmtInfo *
edbss_extract_stmt_info(const char* query_str, int query_len) {
int prefix_len = strlen(EDB_STMT_MAGIC_PREFIX);
const char *info_str = query_str;
int info_len = 0;

if (query_len <= prefix_len)
return NULL;

if (strncmp(info_str, EDB_STMT_MAGIC_PREFIX, prefix_len) == 0) {
const char *c;
info_str += 3; // skip "-- "
c = info_str;
while (*c != '\n' && info_len + 3 < query_len) {
c++;
info_len++;
}
}
int info_len = query_len;
const char *info_str = edbss_extract_info_line(query_str, &info_len);

if (info_len > 0) {
if (info_str) {
EdbStmtInfo *info = (EdbStmtInfo *) palloc0(sizeof(EdbStmtInfo));
EdbStmtInfoSemState state = {
.info = info,
.state = EDB_STMT_INFO_PARSE_NOOP,
};
JsonLexContext *lex = makeJsonLexContextCstringLen(
#if PG_VERSION_NUM >= 170000
NULL,
info_str,
#else
(char *) info_str, // not actually mutating
#endif
info_len,
PG_UTF8,
true);
JsonSemAction sem = {
.semstate = (void *) &state,
.object_start = edbss_json_struct_start,
Expand All @@ -608,14 +603,31 @@ edbss_extract_stmt_info(const char* query_str, int query_len) {
.object_field_start = edbss_json_ofield_start,
.scalar = edbss_json_scalar,
};
JsonParseErrorType parse_rv = pg_parse_json(lex, &sem);
freeJsonLexContext(lex);

if (parse_rv == JSON_SUCCESS)
if ((state.found & EDB_STMT_INFO_PARSE_REQUIRED) == EDB_STMT_INFO_PARSE_REQUIRED)
if (info->query_id != UINT64CONST(0))
return info;

while (info_str) {
JsonLexContext *lex = makeJsonLexContextCstringLen(
#if PG_VERSION_NUM >= 170000
NULL,
info_str,
#else
(char *) info_str, // not actually mutating
#endif
info_len,
PG_UTF8,
true);
JsonParseErrorType parse_rv = pg_parse_json(lex, &sem);
freeJsonLexContext(lex);

if (parse_rv == JSON_SUCCESS)
Copy link
Member

Choose a reason for hiding this comment

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

Does the state get mutated even if there is an error? I guess that's probably fine?

What happens to info_len on an error? Is it still updated with however much got consumed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does the state get mutated even if there is an error? I guess that's probably fine?

Yes, it'll keep the parsed values and continue to the next line, which I think is fine.

What happens to info_len on an error? Is it still updated with however much got consumed?

On error, info_len will be updated to skip the whole failing line and restart on the next line.

if ((state.found & EDB_STMT_INFO_PARSE_REQUIRED) == EDB_STMT_INFO_PARSE_REQUIRED)
return info->query_id != UINT64CONST(0) ? info : NULL;

info_str += info_len + 1;
Copy link
Member

Choose a reason for hiding this comment

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

The +1 makes me nervous about the case where there isn't a newline at the end of an info line?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if the cases where there are untrusted entries in the query log is that likely, but this is C so I want to be extra careful about our boundary cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, good question. The edbss_extract_info_line() function will tag the \n or the end of the query_str, so this +1 will either skip the \n properly, or go beyond the end of the query_str and cause the next call to edbss_extract_info_line() to return NULL.

Copy link
Member

Choose a reason for hiding this comment

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

Because len is negative, at that point?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes exactly

info_len = query_len - (int)(info_str - query_str);
info_str = edbss_extract_info_line(info_str, &info_len);
state.nested_level = 0;
state.state = EDB_STMT_INFO_PARSE_NOOP;
}
edbss_free_stmt_info(info);
}

Expand Down Expand Up @@ -671,6 +683,13 @@ static JsonParseErrorType
edbss_json_scalar(void *semstate, char *token, JsonTokenType tokenType) {
EdbStmtInfoSemState *state = (EdbStmtInfoSemState *) semstate;
Assert(token != NULL);

if (state->found & state->state) {
pfree(token);
state->state = EDB_STMT_INFO_PARSE_NOOP;
return JSON_SUCCESS;
}

switch (state->state) {
case EDB_STMT_INFO_PARSE_QUERY:
if (tokenType == JSON_TOKEN_STRING) {
Expand Down