Hi Shinya, Thanks for the updated v5. Looked at v5 — the arithmetic is correct, and the tests cover the edges. One nit: in-development patches usually pick OIDs in the 8000-9999 range per bki.sgml, but it's not a big deal — the committer renumbers OIDs anyway.
LGTM otherwise. On Sun, May 31, 2026 at 12:51 PM Shinya Kato <[email protected]> wrote: > > On Thu, May 28, 2026 at 2:08 AM lin teletele <[email protected]> wrote: > > > > Hi Shinya, > > > > Thanks for the patches. I read v4-0001 and have a few small observations > > from going through the arithmetic functions. I tested the suggestions > > below locally on top of v4 — they pass the existing xid regression tests > > and produce identical output on the boundary cases listed in section 3. > > Thanks for the review! > > > 1. xid8pl / xid8mi could reuse the helpers in common/int.h > > ---------------------------------------------------- > > xid8pl currently rolls its own overflow detection on a mixed-sign > > addition: > > > > result = val + (uint64) delta; > > if ((delta > 0 && result < val) || (delta < 0 && result > val)) > > ereport(ERROR, ...); > > > > This is correct, but it's the only place in the tree that takes this > > approach, and the (uint64)-of-a-signed-value plus sign-aware compare > > takes a moment to convince oneself of. common/int.h already provides > > pg_add_u64_overflow / pg_sub_u64_overflow, plus pg_abs_s64 which returns > > uint64 and explicitly handles INT64_MIN, so xid8pl could be written as: > > > > uint64 abs_delta = pg_abs_s64(delta); > > bool overflow; > > > > if (delta >= 0) > > overflow = pg_add_u64_overflow(val, abs_delta, &result); > > else > > overflow = pg_sub_u64_overflow(val, abs_delta, &result); > > > > if (overflow) > > ereport(ERROR, ...); > > > > And xid8mi symmetrically (add/sub swapped): > > > > uint64 abs_delta = pg_abs_s64(delta); > > bool overflow; > > > > if (delta >= 0) > > overflow = pg_sub_u64_overflow(val, abs_delta, &result); > > else > > overflow = pg_add_u64_overflow(val, abs_delta, &result); > > > > if (overflow) > > ereport(ERROR, ...); > > > > This keeps the code inside the standard PG overflow-check idiom, and as > > a side effect handles a delta of INT64_MIN cleanly (pg_abs_s64 returns > > 2^63 in that case without invoking UB). > > Agreed. v5 rewrites xid8pl and xid8mi along the lines you suggested. > > > 2. INT64_MIN boundary in xid8_mi_xid8 > > ---------------------------------------------------- > > In the val1 < val2 branch: > > > > if (val2 - val1 > (uint64) PG_INT64_MAX + 1) > > ereport(ERROR, ...); > > PG_RETURN_INT64(-((int64) (val2 - val1))); > > > > The bound permits val2 - val1 == 2^63 (e.g. '0'::xid8 - > > '9223372036854775808'::xid8). When val2 - val1 == 2^63, the cast > > (int64)(val2 - val1) is implementation-defined (the value doesn't fit in > > int64), and -INT64_MIN is signed overflow (UB). In practice on > > two's-complement targets the answer comes out as INT64_MIN, which is the > > correct value, but it relies on UB. > > > > Pulling out the boundary explicitly keeps the same observable behavior > > without the UB: > > > > uint64 diff = val2 - val1; > > > > if (diff > (uint64) PG_INT64_MAX + 1) > > ereport(ERROR, ...); > > /* diff == 2^63 maps to INT64_MIN */ > > if (diff > (uint64) PG_INT64_MAX) > > PG_RETURN_INT64(PG_INT64_MIN); > > PG_RETURN_INT64(-(int64) diff); > > Fixed. > > > 3. Test coverage > > ---------------------------------------------------- > > The regression tests in xid.sql exercise the positive overflow side > > nicely but miss a few boundaries on the negative side: > > > > -- xid8 - xid8 at the INT64_MIN boundary (#2 above) > > select '0'::xid8 - '9223372036854775808'::xid8; > > > > -- xid8 + int8 / xid8 - int8 with INT64_MAX / INT64_MIN deltas > > select '0'::xid8 + 9223372036854775807::bigint; > > select '0'::xid8 - (-9223372036854775807 - 1)::bigint; > > select '9223372036854775807'::xid8 - (-9223372036854775807 - 1)::bigint; > > > > It would be good to pin those down in the expected output. > > Added in v5. I also threw in '0'::xid8 - '9223372036854775809'::xid8. > > > 4. Documentation > > ---------------------------------------------------- > > v4-0001 adds four user-visible operators but doesn't touch doc/src/sgml/. > > pg_lsn's arithmetic operators are documented in datatype.sgml around the > > "pg_lsn Type" section -- it would be nice for the new xid8 operators to > > get analogous coverage in the nearby xid8 paragraph. > > Done in v5. A paragraph modeled on the pg_lsn one is now placed > immediately after the existing xid8 paragraph in datatype.sgml. > > > As a separate observation (probably better as a follow-up thread rather > > than expanding the scope of this one): xid8 currently has only hash and > > btree opclasses, no BRIN. Since xid8 is strictly monotonic and never > > wraps, BRIN minmax looks like a natural fit -- I'll raise that > > separately if there's interest. > > Agreed it's a natural fit, and it deserves its own thread rather than > expanding this one. > > 0001 carries all of the changes above. 0002 is unchanged from v4 apart > from the rebase. > > > -- > Best regards, > Shinya Kato > NTT OSS Center
