Bad canonicalization for dateranges with 'infinity' bounds
https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-INFINITE has:
Also, some element types have a notion of “infinity”, but that is just
another value so far as the range type mechanisms are concerned.
For example, in timestamp ranges, [today,] means the same thing as [today,).
But [today,infinity] means something different from [today,infinity) —
the latter excludes the special timestamp value infinity.
This does not work as expected for ranges with discrete base types,
notably daterange:
test=> SELECT '[2000-01-01,infinity]'::daterange;
daterange
-----------------------
[2000-01-01,infinity)
(1 row)
test=> SELECT '(-infinity,2000-01-01)'::daterange;
daterange
------------------------
[-infinity,2000-01-01)
(1 row)
This is because "daterange_canonical" makes no difference for 'infinity',
and adding one to infinity does not change the value.
I propose the attached patch which fixes the problem.
Yours,
Laurenz Albe
I wrote:
I propose the attached patch which fixes the problem.
I forgot to attach the patch. Here it is.
Yours,
Laurenz Albe
Attachments:
0001-Don-t-canonicalize-dateranges-with-infinity-bounds.patchtext/x-patch; charset=UTF-8; name=0001-Don-t-canonicalize-dateranges-with-infinity-bounds.patchDownload
From 6bbad0acf3baae3a08d1f911b7017642c8a8afe9 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Thu, 2 May 2019 14:32:27 +0200
Subject: [PATCH] Don't canonicalize dateranges with 'infinity' bounds
Since adding one to infinity doesn't change the value, canonicalization
of dateranges with an explicit '-infinity' as exclusive lower bound
would make it an inclusive bound, while dateranges with 'infinity' as
inclusive upper bound would turn it into an exclusive bound.
---
src/backend/utils/adt/rangetypes.c | 4 ++--
src/test/regress/expected/rangetypes.out | 24 ++++++++++++++++++++++++
src/test/regress/sql/rangetypes.sql | 4 ++++
3 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/rangetypes.c b/src/backend/utils/adt/rangetypes.c
index 72c450c70e..a98f17bb66 100644
--- a/src/backend/utils/adt/rangetypes.c
+++ b/src/backend/utils/adt/rangetypes.c
@@ -1431,13 +1431,13 @@ daterange_canonical(PG_FUNCTION_ARGS)
if (empty)
PG_RETURN_RANGE_P(r);
- if (!lower.infinite && !lower.inclusive)
+ if (!(lower.infinite || DATE_NOT_FINITE(lower.val)) && !lower.inclusive)
{
lower.val = DirectFunctionCall2(date_pli, lower.val, Int32GetDatum(1));
lower.inclusive = true;
}
- if (!upper.infinite && upper.inclusive)
+ if (!(upper.infinite || DATE_NOT_FINITE(upper.val)) && upper.inclusive)
{
upper.val = DirectFunctionCall2(date_pli, upper.val, Int32GetDatum(1));
upper.inclusive = false;
diff --git a/src/test/regress/expected/rangetypes.out b/src/test/regress/expected/rangetypes.out
index accf1e0d9e..60d875e898 100644
--- a/src/test/regress/expected/rangetypes.out
+++ b/src/test/regress/expected/rangetypes.out
@@ -652,6 +652,30 @@ select daterange('2000-01-10'::date, '2000-01-11'::date, '(]');
[01-11-2000,01-12-2000)
(1 row)
+select daterange('-infinity'::date, '2000-01-01'::date, '()');
+ daterange
+------------------------
+ (-infinity,01-01-2000)
+(1 row)
+
+select daterange('-infinity'::date, '2000-01-01'::date, '[)');
+ daterange
+------------------------
+ [-infinity,01-01-2000)
+(1 row)
+
+select daterange('2000-01-01'::date, 'infinity'::date, '[)');
+ daterange
+-----------------------
+ [01-01-2000,infinity)
+(1 row)
+
+select daterange('2000-01-01'::date, 'infinity'::date, '[]');
+ daterange
+-----------------------
+ [01-01-2000,infinity]
+(1 row)
+
-- test GiST index that's been built incrementally
create table test_range_gist(ir int4range);
create index test_range_gist_idx on test_range_gist using gist (ir);
diff --git a/src/test/regress/sql/rangetypes.sql b/src/test/regress/sql/rangetypes.sql
index 55638a85ee..9fdb1953df 100644
--- a/src/test/regress/sql/rangetypes.sql
+++ b/src/test/regress/sql/rangetypes.sql
@@ -165,6 +165,10 @@ select daterange('2000-01-10'::date, '2000-01-20'::date, '(]');
select daterange('2000-01-10'::date, '2000-01-20'::date, '()');
select daterange('2000-01-10'::date, '2000-01-11'::date, '()');
select daterange('2000-01-10'::date, '2000-01-11'::date, '(]');
+select daterange('-infinity'::date, '2000-01-01'::date, '()');
+select daterange('-infinity'::date, '2000-01-01'::date, '[)');
+select daterange('2000-01-01'::date, 'infinity'::date, '[)');
+select daterange('2000-01-01'::date, 'infinity'::date, '[]');
-- test GiST index that's been built incrementally
create table test_range_gist(ir int4range);
--
2.20.1
On Fri, May 3, 2019 at 12:49 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
I propose the attached patch which fixes the problem.
Hi Laurenz,
I agree that the patch makes the code match the documentation. The
documented behaviour seems to make more sense than the code, since
unpatched master gives this nonsense result when it flips the
inclusive flag but doesn't adjust the value (because it can't):
postgres=# select '(-infinity,infinity]'::daterange @> 'infinity'::date;
?column?
----------
f
(1 row)
- if (!upper.infinite && upper.inclusive)
+ if (!(upper.infinite || DATE_NOT_FINITE(upper.val)) && upper.inclusive)Even though !(X || Y) is equivalent to !X && !Y, by my reading of
range_in(), lower.value can be uninitialised when lower.infinite is
true, and it's also a bit hard to read IMHO, so I'd probably write
that as !upper.infinite && !DATE_NOT_FINITE(upper.val) &&
upper.inclusive. I don't think it can affect the result but it might
upset Valgrind or similar.
--
Thomas Munro
https://enterprisedb.com
On Sun, Jul 14, 2019 at 12:44 AM Thomas Munro <thomas.munro@gmail.com> wrote:
Even though !(X || Y) is equivalent to !X && !Y, by my reading of
range_in(), lower.value can be uninitialised when lower.infinite is
true, and it's also a bit hard to read IMHO, so I'd probably write
that as !upper.infinite && !DATE_NOT_FINITE(upper.val) &&
upper.inclusive. I don't think it can affect the result but it might
upset Valgrind or similar.
I take back the bit about reading an uninitialised value (X || Y
doesn't access Y if X is true... duh), but I still think the other way
of putting it is a bit easier to read. YMMV.
Generally, +1 for this patch. I'll wait a couple of days for more
feedback to appear.
--
Thomas Munro
https://enterprisedb.com
On Sun, 2019-07-14 at 15:27 +1200, Thomas Munro wrote:
I take back the bit about reading an uninitialised value (X || Y
doesn't access Y if X is true... duh), but I still think the other
way
of putting it is a bit easier to read. YMMV.Generally, +1 for this patch. I'll wait a couple of days for more
feedback to appear.
I went ahead and committed this using Thomas's suggestion to remove the
parentheses.
Thanks!
Regards,
Jeff Davis
Jeff Davis <pgsql@j-davis.com> writes:
I went ahead and committed this using Thomas's suggestion to remove the
parentheses.
The commit message claims this was back-patched, but I see no back-patch?
(The commit message doesn't seem to have made it to the pgsql-committers
list either, but that's probably an independent issue.)
regards, tom lane
On Thu, 2019-07-18 at 13:56 -0700, Jeff Davis wrote:
I went ahead and committed this using Thomas's suggestion to remove the
parentheses.
Thanks for the review and the commit!
Yours,
Laurenz Albe
On Thu, 2019-07-18 at 17:36 -0400, Tom Lane wrote:
The commit message claims this was back-patched, but I see no back-
patch?
Sorry, I noticed an issue after pushing: we were passing a datum
directly to DATE_NOT_FINITE, when we should have called
DatumGetDateADT() first. I ran through the tests again and now pushed
to all branches.
(The commit message doesn't seem to have made it to the pgsql-
committers
list either, but that's probably an independent issue.)
I was curious about that as well.
Regards,
Jeff Davis
On Thu, Jul 18, 2019 at 05:36:35PM -0400, Tom Lane wrote:
Jeff Davis <pgsql@j-davis.com> writes:
I went ahead and committed this using Thomas's suggestion to remove the
parentheses.The commit message claims this was back-patched, but I see no back-patch?
(The commit message doesn't seem to have made it to the pgsql-committers
list either, but that's probably an independent issue.)
REL_12_STABLE has been missed in the set of branches patched. Could
you fix that as well (including the extra fix b0a7e0f0)?
--
Michael
Greetings,
* Jeff Davis (pgsql@j-davis.com) wrote:
On Thu, 2019-07-18 at 17:36 -0400, Tom Lane wrote:
(The commit message doesn't seem to have made it to the pgsql-
committers
list either, but that's probably an independent issue.)I was curious about that as well.
The whitelists we put in place expire after a certain period of time
(iirc, it's 1 year currently) and then your posts end up getting
moderated.
If you register that address as an alternate for you, you should be
able to post with it without needing to be on a whitelist.
Thanks,
Stephen