Horton

Thread

Commits

  1. Adjust cross-version upgrade tests for seg_out() fix

  2. seg: Fix seg_out() to preserve the upper boundary's certainty indicator

  1. [PATCH] seg: preserve the upper boundary's certainty indicator in seg_out()

    Ewan Young <kdbase.hack@gmail.com> — 2026-06-11T07:03:54Z

    Hi Hackers,

While reviewing contrib/seg I noticed that seg_out() mishandles the
certainty indicator ('<', '>' or '~') on the upper boundary of an
interval, leading to wrong output and even silent data loss for valid
values.

For example, on current master:

    regression=# SELECT '1.5 .. ~2.5'::seg;
        seg
    ------------
     1.5 .. 2.5            -- the '~' on the upper bound is dropped

    regression=# SELECT '~6.5 .. 8.5'::seg;
       seg
    ----------
     ~6.5 ..               -- the upper bound 8.5 is lost entirely

The culprit is in seg_out() (contrib/seg/seg.c), where the code that
prints the upper boundary's indicator is:

    if (seg->u_ext == '>' || seg->u_ext == '<' || seg->l_ext == '~')
        p += sprintf(p, "%c", seg->u_ext);

The third test should examine u_ext, not l_ext -- it's a copy-and-paste
slip from the symmetric block that prints the lower boundary a few lines
above (which correctly tests l_ext three times).

This produces two distinct misbehaviours:

  * A '~' on the upper boundary fails the (wrong) condition and is not
    printed at all -> the indicator is dropped.

  * When the lower boundary carries '~' but the upper boundary has no
    indicator (u_ext == '\0'), the wrong test matches and
    sprintf(p, "%c", seg->u_ext) writes a NUL byte into the output
    buffer.  PG_RETURN_CSTRING then stops at that NUL, truncating the
    string and dropping the upper boundary value -> data loss.

Certainty indicators are documented to be preserved on output (they are
ignored by the operators, but kept as a comment), so this breaks the
input/output round-trip for the affected values.

The bug appears to date back to when seg was first added.  It went
unnoticed because the existing regression tests only exercise certainty
indicators on single-point segs (e.g. '~6.5'), which are printed by a
different branch of seg_out() and never reach the buggy line.

The attached patch fixes the one-character typo and adds regression
tests that place each indicator on both boundaries of an interval, so
the upper-boundary case is now covered.  make installcheck passes with
the fix and fails without it.

Regards,
Ewan Young

  2. Re: [PATCH] seg: preserve the upper boundary's certainty indicator in seg_out()

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-06-11T09:45:32Z

    On 11/06/2026 10:03, Ewan Young wrote:
> Certainty indicators are documented to be preserved on output (they are
> ignored by the operators, but kept as a comment), so this breaks the
> input/output round-trip for the affected values.

As a side note, while ignoring the boundaries makes sense for comparison 
operators, seg_union() and seg_intersect() need to do with them. That's 
not documented anywhere, and their current behavior seems pretty 
arbitrary. We haven't actually documented those functions at all, I 
think they were added just for the GiST support and calling them 
directly from SQL was an afterthought.

> The attached patch fixes the one-character typo and adds regression
> tests that place each indicator on both boundaries of an interval, so
> the upper-boundary case is now covered.  make installcheck passes with
> the fix and fails without it.

Applied to master and all stable branches, thanks!

- Heikki

  3. Re: [PATCH] seg: preserve the upper boundary's certainty indicator in seg_out()

    Andrew Dunstan <andrew@dunslane.net> — 2026-06-12T18:58:52Z

    On 2026-06-11 Th 5:45 AM, Heikki Linnakangas wrote:
> On 11/06/2026 10:03, Ewan Young wrote:
>> Certainty indicators are documented to be preserved on output (they are
>> ignored by the operators, but kept as a comment), so this breaks the
>> input/output round-trip for the affected values.
>
> As a side note, while ignoring the boundaries makes sense for 
> comparison operators, seg_union() and seg_intersect() need to do with 
> them. That's not documented anywhere, and their current behavior seems 
> pretty arbitrary. We haven't actually documented those functions at 
> all, I think they were added just for the GiST support and calling 
> them directly from SQL was an afterthought.
>
>> The attached patch fixes the one-character typo and adds regression
>> tests that place each indicator on both boundaries of an interval, so
>> the upper-boundary case is now covered.  make installcheck passes with
>> the fix and fails without it.
>
> Applied to master and all stable branches, thanks!


This is upsetting cross version upgrade tests, I assume since we didn't 
backport it to branches older than 14.

Not sure what the best solution is. Drop the table, or at least the 
offending row?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  4. Re: [PATCH] seg: preserve the upper boundary's certainty indicator in seg_out()

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-06-12T19:02:42Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> This is upsetting cross version upgrade tests, I assume since we didn't 
> backport it to branches older than 14.
> Not sure what the best solution is. Drop the table, or at least the 
> offending row?

I'd vote for teaching AdjustUpgrade.pm to delete just the problematic
row.  No reason to remove more test surface than we have to.

			regards, tom lane

  5. Re: [PATCH] seg: preserve the upper boundary's certainty indicator in seg_out()

    Andrew Dunstan <andrew@dunslane.net> — 2026-06-12T20:44:25Z

    On 2026-06-12 Fr 3:02 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> This is upsetting cross version upgrade tests, I assume since we didn't
>> backport it to branches older than 14.
>> Not sure what the best solution is. Drop the table, or at least the
>> offending row?
> I'd vote for teaching AdjustUpgrade.pm to delete just the problematic
> row.  No reason to remove more test surface than we have to.
>


WFM, I will work on it.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com