Cleanup: Replace sscanf with strtol/strtoul in snapmgr

First seen: 2026-04-20 05:06:21+00:00 · Messages: 5 · Participants: 3

Latest Update

2026-05-06 · opus 4.7

Cleanup: Replace sscanf with strtol/strtoul in snapmgr

Context and Motivation

This thread concerns a focused refactoring patch targeting ImportSnapshot and its helper parsers (parseIntFromText, parseXidFromText, parseVxidFromText) in src/backend/utils/time/snapmgr.c. These helpers parse the text-format snapshot files written by pg_export_snapshot() and read by SET TRANSACTION SNAPSHOT. Each line of such a file has a simple key:value structure containing xids, xid lists, and a virtual transaction id (vxid) formatted as procno/lxid.

The existing implementation uses sscanf() for each field. While correct, sscanf() has two well-known drawbacks in this kind of line-at-a-time parser:

  1. No cheap way to advance past what was consumed. sscanf does not by default report how many characters it consumed (absent the %n conversion), so the calling code has to separately scan forward for the next newline to locate the next record. This amounts to re-scanning bytes that were already examined by the parser.
  2. Weaker error semantics. sscanf conflates "no conversion" and "partial conversion"; detecting overflow requires checking errno in a way that is not uniformly portable, and the integer conversion specifiers (%d, %u, %lu) silently accept leading whitespace and do not distinguish between "nothing there" and "garbage after the number".

The patch converts these helpers to use strtol/strtoul, relying on the endptr out-parameter so the caller can both (a) validate that at least one digit was consumed and (b) resume parsing from the returned pointer without an additional linear scan for the next delimiter.

Why This Matters Architecturally

Snapshot import/export is on the path of parallel query (workers import the leader's snapshot via pg_export_snapshot machinery internally) and of user-driven repeatable-read coordination across sessions. It's not a hot path in the sense of per-tuple execution, but it is security-sensitive: the snapshot file is read from pg_snapshots/ and its contents directly influence visibility. Adopting the idiomatic, end-pointer-driven parsing style used elsewhere in the backend (e.g., GUC parsing, pg_lsn_in, array input functions) reduces the cognitive surface area for auditing this code and removes a small, avoidable O(n) rescan per field.

The patch is therefore explicitly framed as cleanup/alignment rather than a bug fix — the submitter, Amul Sul, positions it as (1) efficiency via endptr reuse and (2) consistency with the rest of the backend's parsing conventions.

Design Discussion

Preservation of existing leniency

Andreas Karlsson (reviewer) explicitly flagged a subtle semantic point: the old sscanf-based code silently ignored trailing garbage on each line, and the new strtol-based code preserves that behavior. He characterized this as "for good and bad" — it is lenient input handling which could mask corrupt or tampered snapshot files, but tightening it would be a behavior change outside the scope of a cleanup patch. The consensus was to keep behavior bit-identical.

Whitespace handling equivalence

Tristan Partin verified an easy-to-miss equivalence: sscanf(" 45", "%d", ...) skips leading whitespace, and strtol/strtoul also skip leading whitespace by definition (per C99 7.20.1.4). So switching parsers does not introduce a regression for lines that happen to have leading spaces. This is the kind of check that typically gets missed in "obvious" parser swaps, and explicitly confirming it in-thread is what moved the patch toward committable status.

Variable naming bikeshed (with substance)

Tristan suggested renaming the locally-declared integers in parseVxidFromText from short forms to procNumber and localTransactionId to match the domain vocabulary used elsewhere (recall that BackendId was renamed to ProcNumber in the PG17 cycle, and VirtualTransactionId contains backendId/localTransactionId fields — now procNumber/localTransactionId). Amul pushed back mildly, preferring procno and xid for brevity, and extended the pattern to parseXidFromText (renaming val → xid) while keeping val in the generic parseIntFromText. This is a defensible compromise: procno matches the procNumber field's conventional abbreviation used in many comments, and keeping val for the generic helper signals that it is field-type-agnostic.

What the Patch Actually Changes

At the level of each helper:

The net effect is shorter code, one fewer pass over each line, and parsing that participates in the usual endptr/errno error-reporting idiom already used across the backend.

Review Trajectory

The patch is small, isolated, and non-behavioral. It drew two reviewers — Andreas Karlsson (general +1 after manual review, noting the preserved leniency) and Tristan Partin (naming suggestion, whitespace-equivalence verification). After the naming revision in v2, Tristan confirmed the diff was limited to the rename and gave a second LGTM. The thread reads as ready-for-commit; no committer has yet applied it within the captured messages, but there are no open objections.

Risks and Residual Concerns

  1. Overflow detection: strtoul returns ULONG_MAX on overflow and sets errno = ERANGE. On 64-bit platforms this is a wider range than the 32-bit xid/procno fields, so the helpers should (and presumably do, though not shown in-thread) additionally range-check against UINT32_MAX or similar. Neither reviewer raised this explicitly, which is a minor gap in the review.
  2. Locale sensitivity: strtol respects LC_NUMERIC only in its hex/octal prefix handling behavior in some libcs; base-10 parsing is locale-independent in practice. This matches sscanf's base-10 behavior, so no regression.
  3. Leniency preservation: As noted, silently ignoring trailing garbage is preserved; hardening is left for a potential future patch.