[PATCH] psql: Make ParseVariableDouble reject values above max

Overview

This is a single-message bug report from Sven Klemm (TigerData, i.e. Timescale) identifying a correctness bug in ParseVariableDouble() in src/bin/psql/variables.c. The bug is narrow in blast radius but is a clear contract violation in a utility function, and it demonstrates a classic class of input-validation mistake: asymmetric handling of the two sides of a bounds check.

The Core Problem

ParseVariableDouble() is psql's helper for parsing a double-precision value from a string-valued psql variable, with optional [min, max] range validation. Its documented contract has two parts:

1. The value must lie within [min, max] to be considered valid.
2. On failure, *result must not be clobbered (so the caller's prior value is preserved).

The bug is that the upper-bound path is implemented as:

• Log the "must be less than N" error message.
• Fall through to the common tail that assigns *result = parsed_value and returns true.

Meanwhile, the lower-bound path correctly returns false immediately. This asymmetry means:

• The caller sees true (success) despite an error message being printed to stderr.
• *result is overwritten with the out-of-range value.
• Any hook that uses the common "validate via ParseVariableDouble → reject assignment if it returns false" idiom is defeated for the upper bound only.

Why It Matters Architecturally

psql's variable machinery (SetVariableHooks, assign/substitute hooks) is built around the convention that a hook returns false to reject an assignment, which causes psql to refuse to update the variable and leave the previous value in place. ParseVariableDouble() is a thin helper meant to encapsulate that validation for numeric variables.

When the helper's success/failure signal disagrees with the error message it prints, every hook built on top of it inherits the bug. Today the only caller is watch_interval_hook (which backs the WATCH_INTERVAL psql variable used by \watch), so the user-visible damage is limited: \watch ends up using an absurdly large interval that psql itself just declared invalid. But the bug is latent for any future caller (e.g., a FETCH_COUNT-style numeric variable, or extensions to \watch_min_rows-style controls).

The reproducer is unambiguous:

# \set WATCH_INTERVAL 99999999
invalid value "99999999" for variable "WATCH_INTERVAL": must be less than 1000000.00
# \echo :WATCH_INTERVAL
99999999

The error message and the resulting state are contradictory — a textbook symptom of a missing return false after the pg_log_error call.

Likely Fix

The minimal fix is a one-line change: after emitting the upper-bound error, return false (mirroring the lower-bound branch) so the common tail that assigns *result and returns true is not reached. The fix should also arguably leave *result untouched on the upper-bound failure path to honor the "isn't clobbered" half of the contract — which a pure return false before the assignment achieves naturally.

A secondary consideration is whether the strtod parse-error path and the NaN/Inf handling obey the same contract; any comprehensive patch would audit all exit paths in the function for symmetric behavior.

Backpatch Considerations

WATCH_INTERVAL was introduced relatively recently (PG17 timeframe), so the bug exists in all branches that contain the variable. Because the fix is tiny, localized to psql, and the current behavior is demonstrably wrong (contract violation with a user-visible inconsistency), it is a natural candidate for backpatching to all supported branches that contain ParseVariableDouble().

Assessment of the Report

The report is well-formed for a hackers bug submission:

• It cites the exact function and file.
• It quotes the documented contract from the function's own comment and shows the two ways it is violated (wrong return value and clobbered *result).
• It provides a minimal reproducer against psql.
• It correctly scopes the user-visible impact (only WATCH_INTERVAL today) without overstating severity.

Sven Klemm is a known contributor (TimescaleDB core developer), which lends credibility to the diagnosis, though the bug is simple enough to verify from first principles by reading the function.