Re: Use pg_current_xact_id() instead of deprecated txid_current()

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 <shinya11(dot)kato(at)gmail(dot)com> wrote:
>
> On Thu, May 28, 2026 at 2:08 AM lin teletele <teletele(dot)lin(at)gmail(dot)com> 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