btree_gin: Incorrect leftmost interval value
In contrib/btree_gin, leftmostvalue_interval() does this:
leftmostvalue_interval(void)
{
Interval *v = palloc(sizeof(Interval));
v->time = DT_NOBEGIN;
v->day = 0;
v->month = 0;
return IntervalPGetDatum(v);
}
which is a long way short of the minimum possible interval value.
As a result, a < or <= query using a GIN index on an interval column
may miss values. For example:
CREATE EXTENSION btree_gin;
CREATE TABLE foo (a interval);
INSERT INTO foo VALUES ('-1000000 years');
CREATE INDEX foo_idx ON foo USING gin (a);
SET enable_seqscan = off;
SELECT * FROM foo WHERE a < '1 year';
a
---
(0 rows)
Attached is a patch fixing this by setting all the fields to their
minimum values, which is guaranteed to be less than any other
interval.
Note that this doesn't affect the contents of the index itself, so
reindexing is not necessary.
Regards,
Dean
Attachments:
fix-leftmostvalue_interval.patchtext/x-patch; charset=US-ASCII; name=fix-leftmostvalue_interval.patchDownload
diff --git a/contrib/btree_gin/btree_gin.c b/contrib/btree_gin/btree_gin.c
new file mode 100644
index c50d68c..b09bb8d
--- a/contrib/btree_gin/btree_gin.c
+++ b/contrib/btree_gin/btree_gin.c
@@ -306,9 +306,9 @@ leftmostvalue_interval(void)
{
Interval *v = palloc(sizeof(Interval));
- v->time = DT_NOBEGIN;
- v->day = 0;
- v->month = 0;
+ v->time = PG_INT64_MIN;
+ v->day = PG_INT32_MIN;
+ v->month = PG_INT32_MIN;
return IntervalPGetDatum(v);
}
diff --git a/contrib/btree_gin/expected/interval.out b/contrib/btree_gin/expected/interval.out
new file mode 100644
index 1f6ef54..8bb9806
--- a/contrib/btree_gin/expected/interval.out
+++ b/contrib/btree_gin/expected/interval.out
@@ -3,30 +3,34 @@ CREATE TABLE test_interval (
i interval
);
INSERT INTO test_interval VALUES
+ ( '-178000000 years' ),
( '03:55:08' ),
( '04:55:08' ),
( '05:55:08' ),
( '08:55:08' ),
( '09:55:08' ),
- ( '10:55:08' )
+ ( '10:55:08' ),
+ ( '178000000 years' )
;
CREATE INDEX idx_interval ON test_interval USING gin (i);
SELECT * FROM test_interval WHERE i<'08:55:08'::interval ORDER BY i;
i
--------------------------
+ @ 178000000 years ago
@ 3 hours 55 mins 8 secs
@ 4 hours 55 mins 8 secs
@ 5 hours 55 mins 8 secs
-(3 rows)
+(4 rows)
SELECT * FROM test_interval WHERE i<='08:55:08'::interval ORDER BY i;
i
--------------------------
+ @ 178000000 years ago
@ 3 hours 55 mins 8 secs
@ 4 hours 55 mins 8 secs
@ 5 hours 55 mins 8 secs
@ 8 hours 55 mins 8 secs
-(4 rows)
+(5 rows)
SELECT * FROM test_interval WHERE i='08:55:08'::interval ORDER BY i;
i
@@ -40,12 +44,14 @@ SELECT * FROM test_interval WHERE i>='08
@ 8 hours 55 mins 8 secs
@ 9 hours 55 mins 8 secs
@ 10 hours 55 mins 8 secs
-(3 rows)
+ @ 178000000 years
+(4 rows)
SELECT * FROM test_interval WHERE i>'08:55:08'::interval ORDER BY i;
i
---------------------------
@ 9 hours 55 mins 8 secs
@ 10 hours 55 mins 8 secs
-(2 rows)
+ @ 178000000 years
+(3 rows)
diff --git a/contrib/btree_gin/sql/interval.sql b/contrib/btree_gin/sql/interval.sql
new file mode 100644
index e385158..7a2f3ac
--- a/contrib/btree_gin/sql/interval.sql
+++ b/contrib/btree_gin/sql/interval.sql
@@ -5,12 +5,14 @@ CREATE TABLE test_interval (
);
INSERT INTO test_interval VALUES
+ ( '-178000000 years' ),
( '03:55:08' ),
( '04:55:08' ),
( '05:55:08' ),
( '08:55:08' ),
( '09:55:08' ),
- ( '10:55:08' )
+ ( '10:55:08' ),
+ ( '178000000 years' )
;
CREATE INDEX idx_interval ON test_interval USING gin (i);
On 27/10/2023 12:26, Dean Rasheed wrote:
In contrib/btree_gin, leftmostvalue_interval() does this:
leftmostvalue_interval(void)
{
Interval *v = palloc(sizeof(Interval));v->time = DT_NOBEGIN;
v->day = 0;
v->month = 0;
return IntervalPGetDatum(v);
}which is a long way short of the minimum possible interval value.
Good catch!
Attached is a patch fixing this by setting all the fields to their
minimum values, which is guaranteed to be less than any other
interval.
LGTM. I wish extractQuery could return "leftmost" more explicitly, so
that we didn't need to construct these leftmost values. But I don't
think that's supported by the current extractQuery interface.
--
Heikki Linnakangas
Neon (https://neon.tech)
On Fri, Oct 27, 2023 at 2:57 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
In contrib/btree_gin, leftmostvalue_interval() does this:
leftmostvalue_interval(void)
{
Interval *v = palloc(sizeof(Interval));v->time = DT_NOBEGIN;
v->day = 0;
v->month = 0;
return IntervalPGetDatum(v);
}which is a long way short of the minimum possible interval value.
As a result, a < or <= query using a GIN index on an interval column
may miss values. For example:CREATE EXTENSION btree_gin;
CREATE TABLE foo (a interval);
INSERT INTO foo VALUES ('-1000000 years');
CREATE INDEX foo_idx ON foo USING gin (a);SET enable_seqscan = off;
SELECT * FROM foo WHERE a < '1 year';
a
---
(0 rows)Attached is a patch fixing this by setting all the fields to their
minimum values, which is guaranteed to be less than any other
interval.
Should we change this to call INTERVAL_NOBEGIN() to be added by
infinite interval patches? It's the same effect but looks similar to
leftmostvalue_timestamp/float8 etc. It will need to wait for the
infinite interval patches to commit but I guess, the wait won't be too
long and the outcome will be better. I can include this change in
those patches.
--
Best Wishes,
Ashutosh Bapat
On Fri, 27 Oct 2023 at 13:56, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
Should we change this to call INTERVAL_NOBEGIN() to be added by
infinite interval patches?
Given that this is a bug that can lead to incorrect query results, I
plan to back-patch it, and INTERVAL_NOBEGIN() wouldn't make sense in
back-branches that don't have infinite intervals.
Regards,
Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
On Fri, 27 Oct 2023 at 13:56, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:Should we change this to call INTERVAL_NOBEGIN() to be added by
infinite interval patches?
Given that this is a bug that can lead to incorrect query results, I
plan to back-patch it, and INTERVAL_NOBEGIN() wouldn't make sense in
back-branches that don't have infinite intervals.
Agreed. When/if the infinite interval patch lands, it could update
this function to use the macro.
regards, tom lane