Let the regex gate see set_config() - #1327
Conversation
01186db to
a996b68
Compare
| /// a function call inside a `SELECT`, so it never matches the statement-start | ||
| /// patterns above. Without it here the parser never sees the statement, and the | ||
| /// interception in `QueryParser::set_config` cannot run at all. | ||
| static CMD_SET_CONFIG: &[&str] = &[r"(?i)\bset_config\b"]; |
There was a problem hiding this comment.
I would be more comfortable if this regex was more like:
SELECT set_config
because the \bset_config\b scan is relatively expensive on long queries and will return nothing 99.99% of the time, while still running in O(n) time (I think).
There was a problem hiding this comment.
You're right that it's very expensive, so i did a benchmark to learn and experiment .
Every pattern in CMD_BASE is ^-anchored through COMMENT_PREFIX, so CMD_RE rejects a non-matching query at offset 0 without reading it. Adding any unanchored alternative removes that and the whole set starts scanning. I measured it in a standalone crate with regex pinned to =1.12.4, the patterns copied verbatim and the same 1000-byte limit, over three non-matching queries (673B, 56B, and a offset_configuration near-miss):
base: anchored patterns only 8.0 ns/query
set_config folded into the set 295.5 ns/query <- this PR as written
"SELECT set_config" folded into the set 235.3 ns/query <- your suggestion
So your version is about 20% cheaper than mine, but both are ~30x the baseline. The literal doesn't get you out of the scan, because it's unanchored too.
It also drops four forms that reach set_config:
\bset_config\b |
SELECT set_config |
|
|---|---|---|
SELECT set_config('a','b',false) |
match | match |
SELECT pg_catalog.set_config(...) |
match | no |
select + 2 spaces + set_config(...) |
match | no |
SELECT\n set_config(...) |
match | no |
SELECT 1, set_config(...) |
match | no |
SELECT offset_configuration FROM t |
no | no |
The pg_catalog. one is the form the Python test in #1298 uses, so that combination would go red on rebase.
The guard
Answering what you actually asked: yes, and it's most of the cost back. Keep both sets anchored-only and test set_config as its own Regex, after the set has already said no:
base set, then standalone Regex 49.1 ns/query
Same results on all 12 inputs I tried, positive and negative. The reason it's 6x cheaper than folding the identical pattern into the set is that RegexSet unions everything into one automaton and loses the literal prefilter; as its own Regex it keeps the SIMD substring scan for set_config. I also tried an explicit aho-corasick pre-check in front of it and it came out at 51.7 ns — no better, so there's no reason to take the dependency.
Two things on the remaining 41 ns. It's bounded by truncate_utf8(query.query(), self.limit), so it's O(min(n, regex_parser_limit)) — 1000 bytes by default, not O(n), and it doesn't degrade on long queries. And CMD_RE_ADVISORY measures 292.6 ns on the same inputs today, so at session_control_and_locks this is about a sixth of what the gate already costs.
Shape:
static SET_CONFIG_RE: Lazy<Regex> = Lazy::new(|| Regex::new(r"(?i)\bset_config\b").unwrap());let prefix = truncate_utf8(query.query(), self.limit);
let cmds = if with_locks { &*CMD_RE_ADVISORY } else { &*CMD_RE };
return cmds.is_match(prefix) || SET_CONFIG_RE.is_match(prefix);Happy to push that, and to post the benchmark source if you want to rerun it — it's about 100 lines and has no dependency on the pgdog tree.
There was a problem hiding this comment.
Can you anchor it? We should make sure it doesn't scan the whole statement if it doesn't match the start of the query.
SELECT set_config(...) changes session state exactly like SET, but it is a function call inside a SELECT rather than a statement-start keyword, so it matches none of the patterns in CMD_BASE. The regex fast path therefore never hands the statement to the parser, and the interception in QueryParser::set_config cannot run at all. That gate is consulted at session_control and session_control_and_locks unconditionally, and at auto whenever the cluster doesn't already force the parser on -- Cluster::router_needed() is false for a single shard that is primary-only or replica-only, which is the plain unsharded deployment. On those a session-scoped set_config() survives checkin and the next client inherits it. Check for it in two steps instead of adding a pattern to the RegexSets. Every pattern in CMD_BASE is anchored, so both sets reject a non-matching query at offset 0 without reading it; folding in an unanchored alternative removes that and costs roughly 35x on every query that does not match. SET_CONFIG_LEAD keeps the anchored bail, and SET_CONFIG_RE runs only after it -- as its own Regex it also keeps the literal prefilter that a RegexSet gives up. SELECT, WITH and a leading paren are the only things that can begin a statement the parser will act on, since extract_set_config takes a SelectStmt whose target list is exactly one set_config call. Forms it declines anyway (EXPLAIN, PREPARE, CREATE TABLE AS, COPY, INSERT, UPDATE) are gated out here at no cost in coverage, and test_set_config_anchored pins that so the two stay in step.
a996b68 to
20e577b
Compare
|
Independent numbers on the anchoring question, since it's a performance argument Benchmark and corpus generator: The corpus turned out to matter more than I expected. My first attempt used an On 2089 statements truncated at Against the real sample rather than the generator: 17.8 / 446.6 / 443.4 / 427.0 Anchoring was worth trying and it doesn't pay. I built The literal is cheaper than the word-boundary version, but only just — 593 What actually moves the number is keeping the pattern out of the So the standalone- |
|
Thank you for comment , the point about statement length dominating Your harness measures five variants, and the shape currently on the PR isn't one SET_CONFIG_LEAD = COMMENT_PREFIX + r"[(\s]*(?:SELECT|WITH)\b"
SET_CONFIG_RE = r"(?i)\bset_config\b"
cmds.is_match(prefix) || (LEAD.is_match(prefix) && RE.is_match(prefix))Because the anchor never spans the statement, the SIMD scan on the second regex Apple silicon rather than WSL2, so these run about 2.4x faster than yours in Split by traffic shape, since that's the whole difference:
D is flat. F is 2.2x cheaper when the anchor bails and ~17% dearer when it On coverage, F doesn't give anything up:
So "both slower and narrower" is fair against the One correction to my own earlier table, which your list inherited: Either shape is fine by me — 5 ns on a path already spending 420 on advisory |
Follow-up to the discussion on #1298, opened separately at @IgorOhrimenko's suggestion since
regex_parser.rsisn't a file that PR touches.The problem
SELECT set_config(...)changes session state exactly likeSET, but it's a function call inside aSELECTrather than a statement-start keyword, so it matches none of the patterns inCMD_BASE. The regex fast path never hands the statement to the parser, so the interception inQueryParser::set_configcan't run — no matter how well it handles the statement once it gets there.Where that bites:
session_control/session_control_and_locks, unconditionally — those levels consult only the gate.auto, whenever the cluster doesn't already force the parser on.Cluster::router_needed()is false for a single shard that is primary-only or replica-only, so a plain unsharded deployment with no read/write split falls through to the gate.On those, a session-scoped
set_config()survives checkin and the next client inherits it. Adding a replica to an otherwise identical config makes it go away, which is what makes this easy to miss.The change
One unanchored pattern. Chaining it into
cmd_base_patterns()rather thanCMD_ADVISORYputs it in bothCMD_REandCMD_RE_ADVISORY—set_config()is session control, not a lock, so it shouldn't sit behind the locks-only level. The\banchors keep it from matching identifiers that merely contain the word;SELECT offset_configuration FROM tis in the test as the negative case.Verification
test_set_configcovers the literal form, the$1bind form,pg_catalog.set_config, and a comment-prefixed variant, atSessionControl,SessionControlAndLocksandAuto. Theregex_parsermodule is 17/17.I measured the underlying behaviour against a
pool_size = 1cluster withpg_backend_pid()compared between clients andlog_statement=allto see what actually reached the server; details are in the comment on #1298.This should also turn the
pgdog_leak_autocases in #1298 green, which is where it came from.One note on running the tests locally
I ran the unit tests at
9918e963rather than atbafec81f.regex_parser.rsis byte-identical between the two, but current main doesn't link on macOS/arm64: since #1324 madepg_raw_parsenon-optional, the build fails withThe generated
wrap_static_fns.cin the build directory is 10 lines and doesn't mentionraw_expression_tree_walker_impl, so bindgen isn't emitting a wrapper for it on that platform — presumably astatic inlinethat clang handles differently there than on the Linux CI images. Unrelated to this change, andcargo build --libis fine; only linking an executable fails. Happy to open a separate issue with the details if that's useful.