Re: query_int wrongly parses zero-padded input

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Still, we should consider whether the right answer is to fix that logic
> to allow hex input. A (weak) argument for going that way is we'd stay
> bug-compatible with the existing behavior for octal input.

I ran a differential sweep to put numbers on that choice: unpatched master
(base 0) against master + Marko's patch (base 10), same server binaries,
only _int.so differs, ~264k parsed inputs. Full write-up and repro are
attached (evidence.txt); the parts that bear on the decision:

Hex and 0b are unreachable, measured. gettoken() copies only '0'-'9' and a
leading '-' into the operand buffer, so strtol never sees 'x' or 'b'. On
unpatched master every one of 0x10, 0X1F, 0b1, 0o7 is already a syntax
error. (_int.so here links __isoc23_strtol, so base 0 *would* take 0x and
0b if either reached it -- it doesn't.) So there is no existing hex
behavior to stay compatible with: allowing hex would be adding a feature,
not preserving one. The only thing base 0 does today is read a
leading-zero operand as octal.

And that octal reading is the harmful part, not something worth keeping. A
leading-zero operand containing an 8 or 9 is truncated silently, not
rejected: '08' and '09' both parse as 0, '-08' as 0 with the sign dropped,
'018' as 1, and '02147483647' as 9020. That is worse, and quieter, than
the '010' -> 8 in Marko's report. Every
behavior change between the two builds falls in this leading-zero class
(33,883 of ~264k inputs; none outside it). The change is per operand and
does not touch the operators, NOT, parentheses or nesting (4,007 random
boolean queries, exact match in both directions), and @@/~~ stay
consistent across seqscan, gist__int_ops, gist__intbig_ops and
gin__int_ops (0 disagreements). Output carries no leading zero, so values
already stored in parsed form survive dump/restore unchanged; only query
text parsed again after the upgrade picks up the decimal reading.

One compatibility point worth a line in the commit message: base 10 does
reject a small set of inputs base 0 accepted -- leading-zero values whose
octal reading fits in int32 but whose decimal reading does not, e.g.
'017777777777' (octal INT_MAX) and '-020000000000' (octal INT_MIN); 5,178
such inputs in the sweep. Nothing goes the other way.

So the measurement lines up with your and Marko's read: base 10 is the
fix, and the hex route would be new behavior rather than kept behavior. If
it helps, tests-suggested.diff (attached, on top of 0001) adds the
silent-truncation and now-rejected cases; it passes on the patched build
and fails on master.

regards,
Manu