[PATCH] seg: preserve the upper boundary's certainty indicator in seg_out()
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
Attachments:
0001-seg-Fix-seg_out-to-preserve-the-upper-boundary-s-cer.patchapplication/octet-stream; name=0001-seg-Fix-seg_out-to-preserve-the-upper-boundary-s-cer.patchDownload+57-2
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
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
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
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