Horton

Thread

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

    Shinya Kato <shinya11.kato@gmail.com> — 2026-05-14T12:31:30Z

    Rebased the patches.

-- 
Best regards,
Shinya Kato
NTT OSS Center

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

    Shinya Kato <shinya11.kato@gmail.com> — 2026-05-25T08:09:47Z

    On Thu, May 14, 2026 at 9:31 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
>
> Rebased the patches.

Rebased the patches, again.

-- 
Best regards,
Shinya Kato
NTT OSS Center

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

    lin teletele <teletele.lin@gmail.com> — 2026-05-27T17:08:46Z

    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.

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).

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);

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.

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.

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.

Regards,
Teletele

On Mon, May 25, 2026 at 4:10 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
>
> On Thu, May 14, 2026 at 9:31 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
> >
> > Rebased the patches.
>
> Rebased the patches, again.
>
> --
> Best regards,
> Shinya Kato
> NTT OSS Center

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

    Shinya Kato <shinya11.kato@gmail.com> — 2026-05-31T04:51:10Z

    On Thu, May 28, 2026 at 2:08 AM lin teletele <teletele.lin@gmail.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