Fix pg_stat_statements display of normalized FETCH counts

First seen: 2026-05-11 06:13:27+00:00 · Messages: 3 · Participants: 2

Latest Update

2026-05-14 · opus 4.7

Core Problem

The thread investigates a perceived inconsistency in pg_stat_statements (PGSS) introduced (or made visible) by the recent feature "Show sizes of FETCH queries as constants in pg_stat_statements". That feature moved FetchStmt.howMany into the part of the parse-tree that is jumble-ignored, so that:

all hash to the same query-id and are accumulated into a single PGSS hash table entry. When the integer count is present, the normalized representative text becomes FETCH $1 c; when absent, the recorded text is the raw FETCH c.

Evan Li's complaint: the representative query text stored in the PGSS entry depends on execution order. If FETCH c (no count) lands first, PGSS records FETCH c as the canonical text even though subsequent FETCH 2 c calls — which would have produced the more informative normalized form FETCH $1 c — are folded into the same entry. Conversely, if a counted FETCH executes first, the nicer normalized text wins.

This is an aesthetic/UX issue rather than a correctness issue at the counter level: calls, timing, and row counts aggregate correctly. But the displayed text can be misleading because FETCH c does not visually convey that FETCH 2 c calls are also counted in the same row.

Proposed Patch

The submitted patch adds a query_normalized boolean flag to pgssEntry. The store path is modified so that:

  1. On insert, the flag records whether the stored text contains a $N substitution (i.e., whether generate_normalized_query() produced the text or whether the raw query string was used because no constants were jumbled out).
  2. On a subsequent hit to the same entry, if the existing entry's text is not normalized but the incoming query is, replace the stored text (and presumably re-point query_offset/query_len into the external query text file).

To avoid regressions, the replacement logic is gated by a new parameter to pgss_store() and enabled only for FetchStmt. The submitter explicitly notes the gating was needed because otherwise statements like SET SESSION AUTHORIZATION 'r1' vs 'r2' — which are not jumble-merged (the string is a constant kept in the jumble) — would be mishandled. The submitter is uncertain whether opt-in (whitelist FETCH) or opt-out (blacklist SET SESSION AUTHORIZATION and friends) is the better policy.

Michael Paquier's Rejection and Counter-Design

Michael Paquier (committer, PGSS area maintainer) rejects both the framing and the implementation:

  1. Semantic framing. The grouping is intentional. After the recent change, FETCH c and FETCH N c both deparse to a forward fetch with FetchDirectionKeyword = FETCH_KEYWORD_NONE; howMany is the only difference and it has been moved into the jumble-ignore region precisely so all forward-fetches collapse together. From the executor/parse-tree perspective they are the same statement shape, and "more normalization is better" for monitoring.

  2. Performance objection. Calling generate_normalized_query() a second time on the entry-update path is unacceptable. generate_normalized_query() does a pass over the source text plus token rewriting and is one of the hotter parts of PGSS; the existing design deliberately invokes it only once, on first insert. Re-running it whenever a "better" text arrives would put it on every insert path that hits an existing entry until the upgrade happens — a measurable regression on a hot extension that is enabled in production almost universally.

  3. Correct fix, if a fix is wanted. If users genuinely want to distinguish FETCH c from FETCH N c, the right place to encode that distinction is in FetchDirectionKeywords itself: introduce a new variant such as FETCH_KEYWORD_SINGLE and assign it in gram.y to the cursor_name and from_in cursor_name productions (the no-count cases). Because FetchDirection is part of the jumble, the two forms would then hash to different query-ids and live in separate PGSS entries naturally — no special-case flag, no re-normalization, no opt-in/opt-out list. Michael does not personally advocate doing this; he considers the current coarser grouping preferable for monitoring.

Key Technical Insights

Why "first query wins" is the right invariant

PGSS's contract is: one entry per (userid, dbid, queryid, toplevel). The representative text is a sample of one query that hashed to that bucket, not a canonical reconstruction. Keeping it immutable after first insert is what makes PGSS cheap: the query text file (pg_stat_statements/) is append-only; rewriting an existing entry's text would require either invalidating its query_offset and appending a new copy (bloating the text file and demanding more aggressive gc_qtexts() cycles) or in-place rewriting (impossible since lengths differ). The submitter's patch does not appear to have grappled with the text-file lifecycle implications.

Why the SET SESSION AUTHORIZATION caveat exposes a deeper issue

The fact that the patch needed a per-statement-kind gate is a strong signal the design is wrong. Whether a statement's representative text "should" be replaceable is not a property of statement kind — it is a property of whether the jumble truly captures all distinguishing information. For FETCH after the recent patch, it does (howMany is ignored). For SET SESSION AUTHORIZATION 'r1', it does not (the role string is a literal that is not normalized out, so two such statements actually have different queryids and never share an entry — meaning the replacement logic wouldn't fire anyway, contrary to the submitter's worry). The submitter's mental model of why his patch breaks SET SESSION AUTHORIZATION is itself slightly off; that is a clue that the abstraction "is this entry's text normalized?" is not the right one.

The grammar-level alternative

Michael's suggested fix is structurally cleaner because it pushes the distinction down to where queryid is computed. The taxonomy of FETCH directions in parsenodes.h already enumerates NONE / FORWARD / BACKWARD / ABSOLUTE / RELATIVE / ALL / FORWARD_ALL / BACKWARD_ALL; adding FETCH_KEYWORD_SINGLE for the count-less default would split the bucket without any special-case code in PGSS. The cost is a small but real loss of aggregation: users who want to know "how often does the application FETCH from cursor c?" would now need to sum two rows.

Participant Dynamics

Evan Li is a contributor exploring PGSS internals; he submits a patch but is candid about its rough edges (the FETCH-only opt-in he is "not fully satisfied with"). Michael Paquier is the de-facto owner of PGSS in the modern tree and his judgment that "this is not something we need to act on" is essentially dispositive. The submitter accepts this gracefully and offers to withdraw, ending the thread within ~24 hours of the original post.

Outcome

The patch is effectively withdrawn. No commit results. The behavior — first-seen query text wins, even when a later query in the same bucket would have produced a more informative normalized form — is reaffirmed as intentional. A potential future improvement (FETCH_KEYWORD_SINGLE in the grammar) is documented for anyone who later argues the aggregation is too coarse.