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
v5-0001-Add-arithmetic-operators-for-xid8.patch
Description: Binary data
v5-0002-Use-pg_current_xact_id-instead-of-deprecated-txid.patch
Description: Binary data
