adapter: Add emergency logging of arriving statements#37567
Conversation
def-
left a comment
There was a problem hiding this comment.
Just want to have it documented, are we ok with unredacted secrets/binds here?
|
Edit 3: Actually, I've just added more redaction, see second commit. |
d0af573 to
8caf725
Compare
def-
left a comment
There was a problem hiding this comment.
Iceberg credentials still make it through apparently:
CREATE CONNECTION polaris TO ICEBERG CATALOG (
CATALOG TYPE = 'REST',
URL = 'https://catalog.example',
CREDENTIAL = 'client-id:sk_live_abc123',
WAREHOUSE = 'warehouse'
);515ac96 to
87cf79c
Compare
|
Thanks @def-, I've added that as well now. So, the full list is "secret", "password", "credential", "token". |
jasonhernandez
left a comment
There was a problem hiding this comment.
We can merge this - we'll need to be careful with its use given where this sits in processing queries.
mtabebe
left a comment
There was a problem hiding this comment.
One question, but the change looks fine for me for what we are trying to accomplish
| } | ||
|
|
||
| /// Reports whether `enable_statement_arrival_logging` is on. | ||
| pub async fn statement_arrival_logging_enabled(&mut self) -> bool { |
There was a problem hiding this comment.
Why do we need to look at this in the catalog on every call why can't we just check the DynCfg?
There was a problem hiding this comment.
I guess it means we don't need a restart to pick it up?
There was a problem hiding this comment.
There's no live dyncfg handle (ConfigSet) available in the frontends or SessionClient today. The only way to read a dyncfg from pgwire is get_system_vars(), which is a coordinator round-trip per call. The catalog snapshot, by contrast, is served from the session's snapshot cache, so in the steady state the check is just a Weak::upgrade plus an atomic revision check, with no round-trip.
(On the restart question: neither mechanism needs one, both dyncfgs and system vars are runtime-updatable. The snapshot is simply the cheapest config handle we already have at this spot.)
edf5deb to
3da7989
Compare
|
I've added one more commit that changes the redaction: We now redact the same way as the statement log: by calling (Another drawback, shared with the statement log, is that the statement often won't be a copy-pasteable repro, because the redaction is not mindful of the types of literals and stuff, so the redacted statement often has syntax errors, see https://github.com/MaterializeInc/database-issues/issues/8796. But I guess in the LLM-era those syntax errors are not hard to fix up.) |
Add the internal system var enable_statement_arrival_logging (default off). When on, the pgwire and HTTP SQL frontends log every incoming statement and other frontend message at info level as soon as it arrives, before any processing. This is an emergency diagnostic for statements that repeatedly crash environmentd before they reach the statement log, or before the statement log is written out to persist. Extended protocol messages log prepared statement and portal names plus bind parameters, so an execute can be connected back to the parse that carried the SQL text, together with the connection id. Authentication payloads are never logged. COPY data is logged as its length only. The logged SQL is not redacted, which is one of the reasons the flag defaults to off. The flag is read through the session's cached catalog snapshot, so the per-message cost while the flag is off is negligible. See https://linear.app/materializeinc/issue/DB-161/can-we-log-redacted-sql-early-at-debug-level-to-aid-panic Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Statement text or bind parameters that might contain a sensitive value are replaced by a placeholder. We sniff for the keywords "secret" and "password" (case-insensitively), since precise redaction would require parsing, and arrival logging runs before parsing on purpose. Bind parameters are also redacted when the prepared statement they refer to trips the check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The CREATE CONNECTION options CREDENTIAL and SESSION TOKEN accept inline sensitive values but contain neither "secret" nor "password". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Instead of the keyword-based skipping of possibly sensitive statements, parse the arriving SQL and log it with its literals redacted, using the same redaction as the statement log. Statements that fail to parse are logged as a placeholder with only their byte length. Since logging now requires parsing, a statement that crashes the parser is not captured at all anymore, an accepted tradeoff. Bind parameter values are data that redaction cannot reach, so only their count is logged now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fix a stale doc comment that still described the keyword-based redaction. Stop logging CopyFail's client-supplied message. Distinguish too-large statements from unparseable ones in the placeholder. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8891550 to
8ce6f96
Compare
Motivation
Fixes DB-161, which came from the stack overflow incident.
An emergency diagnostic for statements that repeatedly crash environmentd before they reach the statement log, or before the statement log is written out to persist (which happens only every few seconds).
Description
Adds the internal system var
enable_statement_arrival_logging(default OFF). When on, the pgwire and HTTP SQL frontends log every incoming statement and other frontend message at info level as soon as it arrives, before any processing beyond parsing.advance_ready, before dispatching on the message. (Messages consumed by the COPY subprotocol or the post-error drain loop are not logged.)executecan be connected back to theparsethat carried the SQL text, together with the connection id and session UUID. Bind parameter values are data that redaction cannot reach, so only their count is logged.FrontendMessagevariant fails compilation until the author decides how to log it.enable_statement_arrival_loggingfollows the pattern ofenable_internal_statement_logging(a plain internal bool system var, not afeature_flags!entry, since it gates no SQL surface).Example log lines with the flag on:
Verification
No tests added. Verified manually against a local environmentd: simple and extended pgwire protocol, HTTP simple and extended requests, and ALTER SYSTEM SET/RESET turning the logging on and off immediately. Also verified the literal redaction (CREATE SECRET values, inline connection credentials such as ICEBERG CREDENTIAL, string and numeric literals in queries), the placeholder for unparseable statements, and that none of the planted sensitive values appear anywhere in the log.
🤖 Generated with Claude Code
(An alternative would be to add a Debug-level log line instead of a new config flag. I rejected this approach, because turning on Debug logging for the entire envd affects performance badly. We could do it per-crate, but then it just gets too complex. Flipping a simple flag is easier to get right during an incident.)