Обсуждение: 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

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