Skip to content

Commit 71a3c16

Browse files
Code review feedback
1 parent f892d8e commit 71a3c16

13 files changed

Lines changed: 364 additions & 156 deletions

File tree

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
SET PERSIST vsql_allow_preview_extensions = ON;
2+
INSTALL EXTENSION vsql_slow_query_log;
3+
SET GLOBAL vsql_slow_query_log.threshold_ms = 0;
4+
SET GLOBAL vsql_slow_query_log.log_file = 'MYSQLTEST_VARDIR/tmp/vsql_slow_ddl.log';
5+
SET GLOBAL vsql_slow_query_log.enabled = ON;
6+
# DDL: CREATE TABLE
7+
CREATE TABLE vsql_hook_ddl_test (id INT PRIMARY KEY, val VARCHAR(32));
8+
# DDL: ALTER TABLE
9+
ALTER TABLE vsql_hook_ddl_test ADD COLUMN extra INT;
10+
# DDL: DROP TABLE
11+
DROP TABLE vsql_hook_ddl_test;
12+
# GRANT
13+
CREATE USER IF NOT EXISTS 'vsql_hook_test_user'@'localhost';
14+
GRANT SELECT ON *.* TO 'vsql_hook_test_user'@'localhost';
15+
# REVOKE
16+
REVOKE SELECT ON *.* FROM 'vsql_hook_test_user'@'localhost';
17+
DROP USER 'vsql_hook_test_user'@'localhost';
18+
SET GLOBAL vsql_slow_query_log.enabled = OFF;
19+
# CREATE TABLE was logged
20+
1
21+
# ALTER TABLE was logged
22+
1
23+
# DROP TABLE was logged
24+
1
25+
# GRANT was logged
26+
1
27+
# REVOKE was logged
28+
1
29+
UNINSTALL EXTENSION vsql_slow_query_log;
30+
SET PERSIST vsql_allow_preview_extensions = OFF;
31+
RESET PERSIST vsql_allow_preview_extensions;
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
# Verify that the POSTEXECUTE query hook fires for DDL and GRANT/REVOKE
2+
# statements, not only for DML. Uses threshold_ms=0 so every statement is
3+
# logged regardless of duration.
4+
5+
--let $slow_log = $MYSQLTEST_VARDIR/tmp/vsql_slow_ddl.log
6+
7+
SET PERSIST vsql_allow_preview_extensions = ON;
8+
9+
--error 0,1
10+
--remove_file $slow_log
11+
12+
INSTALL EXTENSION vsql_slow_query_log;
13+
14+
SET GLOBAL vsql_slow_query_log.threshold_ms = 0;
15+
--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
16+
eval SET GLOBAL vsql_slow_query_log.log_file = '$slow_log';
17+
SET GLOBAL vsql_slow_query_log.enabled = ON;
18+
19+
--echo # DDL: CREATE TABLE
20+
CREATE TABLE vsql_hook_ddl_test (id INT PRIMARY KEY, val VARCHAR(32));
21+
22+
--echo # DDL: ALTER TABLE
23+
ALTER TABLE vsql_hook_ddl_test ADD COLUMN extra INT;
24+
25+
--echo # DDL: DROP TABLE
26+
DROP TABLE vsql_hook_ddl_test;
27+
28+
--echo # GRANT
29+
CREATE USER IF NOT EXISTS 'vsql_hook_test_user'@'localhost';
30+
GRANT SELECT ON *.* TO 'vsql_hook_test_user'@'localhost';
31+
32+
--echo # REVOKE
33+
REVOKE SELECT ON *.* FROM 'vsql_hook_test_user'@'localhost';
34+
DROP USER 'vsql_hook_test_user'@'localhost';
35+
36+
SET GLOBAL vsql_slow_query_log.enabled = OFF;
37+
38+
--echo # CREATE TABLE was logged
39+
--exec sh -c "grep -c 'CREATE TABLE vsql_hook_ddl_test' $slow_log; true"
40+
41+
--echo # ALTER TABLE was logged
42+
--exec sh -c "grep -c 'ALTER TABLE vsql_hook_ddl_test' $slow_log; true"
43+
44+
--echo # DROP TABLE was logged
45+
--exec sh -c "grep -c 'DROP TABLE vsql_hook_ddl_test' $slow_log; true"
46+
47+
--echo # GRANT was logged
48+
--exec sh -c "grep -c 'GRANT SELECT' $slow_log; true"
49+
50+
--echo # REVOKE was logged
51+
--exec sh -c "grep -c 'REVOKE SELECT' $slow_log; true"
52+
53+
UNINSTALL EXTENSION vsql_slow_query_log;
54+
55+
SET PERSIST vsql_allow_preview_extensions = OFF;
56+
RESET PERSIST vsql_allow_preview_extensions;
57+
58+
--remove_file $slow_log

sql/sql_audit.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@
6464
#include "sql_string.h"
6565
#include "strxnmov.h"
6666
#include "thr_mutex.h"
67-
#include "villagesql/services/preview/query_hook.h"
67+
#include "villagesql/services/preview/statement_event.h"
6868

6969
namespace {
7070
/**
@@ -1097,7 +1097,7 @@ int mysql_event_tracking_query_notify(
10971097
mysql_event_tracking_query_data event;
10981098

10991099
if (subclass & EVENT_TRACKING_QUERY_STATUS_END)
1100-
villagesql::services::on_query_status_end(thd);
1100+
villagesql::services::on_statement_postexecute(thd);
11011101

11021102
if (thd->check_event_subscribers(Event_tracking_class::QUERY, subclass, true))
11031103
return 0;

villagesql/examples/vsql-slow-query-log/src/extension.cc

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,22 +34,22 @@
3434
#include <ctime>
3535
#include <mutex>
3636

37-
#include <villagesql/preview/query_hook.h>
37+
#include <villagesql/preview/statement_event.h>
3838
#include <villagesql/preview/sys_var.h>
3939
#include <villagesql/vsql.h>
4040

4141
using namespace vsql;
4242
namespace sv = vsql::preview_sys_var;
43-
namespace qh = vsql::preview_query_hook;
43+
namespace se = vsql::preview_statement_event;
4444

4545
static bool g_enabled;
4646
static long long g_threshold_ms;
4747
static char *g_log_filename;
4848

4949
static std::mutex g_log_mutex;
5050

51-
static void slow_query_hook(const qh::QueryHookArgs &args,
52-
qh::QueryHookResult &result) {
51+
static void slow_query_hook(const se::StatementEventArgs &args,
52+
se::StatementEventResult &result) {
5353
if (!g_enabled) return;
5454
if (args.query_time_secs() * 1000.0 < static_cast<double>(g_threshold_ms))
5555
return;
@@ -98,7 +98,8 @@ static auto SYS_VARS = sv::make_capability({
9898
&g_log_filename, "/tmp/vsql_slow_query.log"),
9999
});
100100

101-
static qh::QueryHookCapability<VEF_QUERY_HOOK_POSTEXECUTE, &slow_query_hook>
101+
static se::StatementEventCapability<VEF_STATEMENT_EVENT_POSTEXECUTE,
102+
&slow_query_hook>
102103
QUERY_HOOK;
103104

104105
VEF_GENERATE_ENTRY_POINTS(make_extension().with(SYS_VARS).with(QUERY_HOOK))

villagesql/sdk/include/villagesql/abi/preview/query_hook.h renamed to villagesql/sdk/include/villagesql/abi/preview/statement_event.h

Lines changed: 77 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@
2323
// notice. See villagesql/preview/README.md.
2424
// =============================================================================
2525

26-
#ifndef VILLAGESQL_ABI_PREVIEW_QUERY_HOOK_H
27-
#define VILLAGESQL_ABI_PREVIEW_QUERY_HOOK_H
26+
#ifndef VILLAGESQL_ABI_PREVIEW_STATEMENT_EVENT_H
27+
#define VILLAGESQL_ABI_PREVIEW_STATEMENT_EVENT_H
2828

2929
#include <stdbool.h>
3030
#include <stddef.h>
@@ -36,19 +36,19 @@
3636
extern "C" {
3737
#endif
3838

39-
// Preview capability: "vsql::preview::query_hook"
39+
// Preview capability: "vsql::preview::statement_event"
4040
//
41-
// An extension declares one QueryHookCapability per hook function. The server
42-
// invokes registered hooks at the corresponding phase of query processing.
43-
// Only the POSTEXECUTE phase is wired up in this version; the other phase
44-
// values are reserved for future use without ABI break.
41+
// An extension declares one StatementEventCapability per hook function. The
42+
// server invokes registered hooks at the corresponding phase of query
43+
// processing. Only the POSTEXECUTE phase is wired up in this version; the other
44+
// phase values are reserved for future use without ABI break.
4545
//
46-
// Capability name: VEF_PREVIEW_QUERY_HOOK_NAME
46+
// Capability name: VEF_PREVIEW_STATEMENT_EVENT_NAME
4747

48-
#define VEF_PREVIEW_QUERY_HOOK_NAME "vsql::preview::query_hook"
48+
#define VEF_PREVIEW_STATEMENT_EVENT_NAME "vsql::preview::statement_event"
4949

5050
// Capability ABI version compiled into this SDK snapshot.
51-
#define VEF_PREVIEW_QUERY_HOOK_ABI_VERSION 1
51+
#define VEF_PREVIEW_STATEMENT_EVENT_ABI_VERSION 1
5252

5353
// Phase at which a hook fires.
5454
//
@@ -61,64 +61,79 @@ typedef enum {
6161
// EVENT_TRACKING_CONNECTION_CONNECT. Args populated: user, host,
6262
// connection_id, port. Query fields and timing are unset.
6363
// Result.error_msg refuses the connection.
64-
VEF_QUERY_HOOK_CONNECT = 0,
64+
VEF_STATEMENT_EVENT_CONNECT = 0,
6565

6666
// Reserved: client connection closing. Fires from
6767
// EVENT_TRACKING_CONNECTION_DISCONNECT. Same args as CONNECT.
6868
// Result is ignored — the connection is going away.
69-
VEF_QUERY_HOOK_DISCONNECT = 1,
69+
VEF_STATEMENT_EVENT_DISCONNECT = 1,
7070

7171
// Reserved: before the parser runs. Only `query` is populated. Hooked
7272
// inside sql_parse.cc rather than the audit path. Result.error_msg
7373
// blocks the query.
74-
VEF_QUERY_HOOK_PREPARSE = 2,
74+
VEF_STATEMENT_EVENT_PREPARSE = 2,
7575

7676
// Reserved: after the parser runs. Adds sql_command, is_prepared, and
7777
// the SHA-256 digest of the normalized query to args. Result.error_msg
7878
// blocks the query.
79-
VEF_QUERY_HOOK_POSTPARSE = 3,
79+
VEF_STATEMENT_EVENT_POSTPARSE = 3,
8080

8181
// Reserved: after parsing, before execution begins. Fires from
8282
// EVENT_TRACKING_QUERY_START. Args populated: query, user, host,
8383
// connection_id, port, schema, sql_command, in_transaction. Timing/rows/
8484
// status are zero. Result.error_msg blocks the query.
85-
VEF_QUERY_HOOK_PREEXECUTE = 4,
85+
VEF_STATEMENT_EVENT_PREEXECUTE = 4,
8686

8787
// After execution completes (success or failure). All args fields are
8888
// populated; result.error_msg is logged but does not affect the client.
89-
VEF_QUERY_HOOK_POSTEXECUTE = 5,
90-
} vef_query_hook_phase_t;
89+
VEF_STATEMENT_EVENT_POSTEXECUTE = 5,
90+
} vef_statement_event_phase_t;
9191

9292
// Read-only arguments passed to a hook invocation. Which fields are populated
9393
// depends on the phase; POSTEXECUTE populates all of them.
94+
//
95+
// Pointer lifetime summary (see per-field comments):
96+
// process lifetime — safe to retain indefinitely; no copy needed.
97+
// connection lifetime — valid until the client disconnects; safe to retain
98+
// across hook calls for the same connection, but copy
99+
// if you may use it after the connection ends.
100+
// copy before return — valid only during this hook invocation; copy the
101+
// string before the hook function returns if you need
102+
// to keep it.
94103
typedef struct {
95-
vef_query_hook_phase_t phase;
104+
vef_statement_event_phase_t phase;
96105

97106
// Query text. Not null-terminated; use query_len.
107+
// Lifetime: copy before return.
98108
const char *query;
99109
size_t query_len;
100110

101-
// Authenticated user name (priv_user), or empty string.
111+
// Authenticated user name, or empty string.
112+
// Lifetime: connection lifetime.
102113
const char *user;
103114
// Client IP address, or empty string.
115+
// Lifetime: connection lifetime.
104116
const char *host;
105117
unsigned long connection_id;
106118
uint16_t port;
107119
bool in_transaction;
108120

109121
// SQL command as a lowercase string (e.g. "select", "insert",
110-
// "create_table"). Stable across MySQL version rebases. NULL if unknown.
111-
// The pointed-to string has static lifetime and need not be copied.
122+
// "create_table"). Stable across MySQL version rebase. NULL if unknown.
123+
// Lifetime: process lifetime.
112124
const char *sql_command;
113125

114-
// Default schema (USE <db>), or NULL if none selected.
126+
// Current default schema (USE <db>), or NULL if none selected.
127+
// Lifetime: connection lifetime.
115128
const char *schema;
116129

117130
// Execution status. 0 on success, otherwise the MySQL error code.
118131
int status;
119132
// 5-character SQLSTATE + NUL, or NULL on success.
133+
// Lifetime: copy before return.
120134
const char *sqlstate;
121135
// Error message text, or NULL on success.
136+
// Lifetime: copy before return.
122137
const char *error_message;
123138

124139
// Query start time, microseconds since epoch.
@@ -130,7 +145,38 @@ typedef struct {
130145
uint64_t rows_affected;
131146
uint64_t bytes_sent;
132147
uint64_t bytes_received;
133-
} vef_query_hook_args_t;
148+
149+
// Number of warnings raised during execution (in addition to any error).
150+
uint64_t warning_count;
151+
152+
// Normalized (digested) form of the query, suitable for grouping similar
153+
// queries regardless of literal values. NULL if digest computation was
154+
// disabled or the query was too long to digest.
155+
// Lifetime: copy before return.
156+
const char *digest_text;
157+
158+
// Optimizer and execution quality indicators. Non-zero values suggest
159+
// potentially inefficient execution.
160+
uint64_t select_full_join; // joins without usable index
161+
uint64_t select_full_range_join; // joins using range on ref table
162+
uint64_t select_range; // range scans on first table
163+
uint64_t select_range_check; // joins with key check per row
164+
uint64_t select_scan; // full scans of first table
165+
166+
// Sort metrics.
167+
uint64_t sort_merge_passes; // number of merge passes (high = large sort)
168+
uint64_t sort_range; // sorts using a range
169+
uint64_t sort_rows; // rows sorted
170+
uint64_t sort_scan; // sorts using a full table scan
171+
172+
// Temporary table usage.
173+
uint64_t created_tmp_tables; // tmp tables created (memory or disk)
174+
uint64_t created_tmp_disk_tables; // tmp tables spilled to disk
175+
176+
// Index usage flags. Non-zero means the query ran without a usable index.
177+
uint8_t no_index_used; // 1 if no index was used
178+
uint8_t no_good_index_used; // 1 if no good index was found
179+
} vef_statement_event_args_t;
134180

135181
// Writable result. For POSTEXECUTE error_msg is advisory: the server logs it
136182
// but does not propagate to the client.
@@ -141,32 +187,32 @@ typedef struct {
141187
// this pointer before calling the hook. Matches the convention used by
142188
// vef_vdf_result_t and vef_prerun_result_t.
143189
char *error_msg;
144-
} vef_query_hook_result_t;
190+
} vef_statement_event_result_t;
145191

146192
// Hook callback type. Invoked synchronously on the query's thread.
147193
// args is read-only; result is the extension's writeback channel.
148-
typedef void (*vef_query_hook_fn_t)(const vef_query_hook_args_t *args,
149-
vef_query_hook_result_t *result);
194+
typedef void (*vef_statement_event_fn_t)(const vef_statement_event_args_t *args,
195+
vef_statement_event_result_t *result);
150196

151197
// Capability config (cc) filled in by the extension and passed to the server
152198
// via vef_required_capability_t.capability_config. The phase determines when
153199
// the hook fires; the function pointer must remain valid for the lifetime of
154200
// the extension.
155201
typedef struct {
156-
vef_query_hook_phase_t phase;
157-
vef_query_hook_fn_t hook;
158-
} vef_query_hook_cc_t;
202+
vef_statement_event_phase_t phase;
203+
vef_statement_event_fn_t hook;
204+
} vef_statement_event_cc_t;
159205

160206
// Server-side vtable. The version field is always first, matching the
161207
// convention used by other preview capabilities. The server exposes no
162208
// methods to the extension for this capability — registration happens via
163209
// capability_config.
164210
typedef struct {
165211
uint32_t version;
166-
} vef_preview_query_hook_t;
212+
} vef_preview_statement_event_t;
167213

168214
#ifdef __cplusplus
169215
}
170216
#endif
171217

172-
#endif // VILLAGESQL_ABI_PREVIEW_QUERY_HOOK_H
218+
#endif // VILLAGESQL_ABI_PREVIEW_STATEMENT_EVENT_H

0 commit comments

Comments
 (0)