Обсуждение: Add missing function abs (interval)
On a newly set up system there are 7 types with a unary minus operator defined, but only 6 of them have an abs function:
postgres=# \df abs
List of functions
Schema | Name | Result data type | Argument data types | Type
------------+------+------------------+---------------------+------
pg_catalog | abs | bigint | bigint | func
pg_catalog | abs | double precision | double precision | func
pg_catalog | abs | integer | integer | func
pg_catalog | abs | numeric | numeric | func
pg_catalog | abs | real | real | func
pg_catalog | abs | smallint | smallint | func
(6 rows)
I now have the following definition in my database:
CREATE OR REPLACE FUNCTION abs (
p interval
) RETURNS interval
LANGUAGE SQL IMMUTABLE STRICT
SET search_path FROM CURRENT
AS $$
SELECT GREATEST (p, -p)
$$;
COMMENT ON FUNCTION abs (interval) IS 'absolute value';
Would a patch to add a function with this behaviour to the initial database be welcome?
If so, should I implement it essentially like the above, or as an internal function? I've noticed that even when it seems like it might be reasonable to implement a built-in function as an SQL function they tend to be internal.
On Mon, Mar 29, 2021 at 3:33 PM Isaac Morland <isaac.morland@gmail.com> wrote:
>
> On a newly set up system there are 7 types with a unary minus operator defined, but only 6 of them have an abs function:
>
...
> Would a patch to add a function with this behaviour to the initial database be welcome?
Looking in the archives, I see this attempt that you can build upon:
> If so, should I implement it essentially like the above, or as an internal function? I've noticed that even when it seems like it might be reasonable to implement a built-in function as an SQL function they tend to be internal.
On Mon, Mar 29, 2021 at 07:15:19PM -0400, John Naylor wrote: > Looking in the archives, I see this attempt that you can build upon: > https://www.postgresql.org/message-id/flat/CAHE3wggpj%2Bk-zXLUdcBDRe3oahkb21pSMPDm-HzPjZxJn4vMMw%40mail.gmail.com I see no problem with doing something more here. If you can get a patch, please feel free to add it to the next commit fest, for Postgres 15: https://commitfest.postgresql.org/33/ -- Michael
Вложения
I've attached a patch for this. Turns out there was a comment in the source explaining that there is no interval_abs because it's not clear what to return; but I think it's clear that if i is an interval the larger of i and -i should be considered to be the absolute value, the same as would be done for any type; essentially, if the type is orderable and has a meaningful definition of unary minus, the definition of abs follows from those.
This does have some odd effects, as was observed in the previous discussion pointed at by John Naylor above (for which thanks!). But those odd effects are not due to abs(interval) itself but rather due to the odd behaviour of interval, where values which compare equal to '0'::interval can change a timestamp when added to it. This in turn comes from what the interval data type is trying to do combined with the inherently complicated nature of our timekeeping system.
I have included in the test case some testing of what happens with '1 month -30 days'::interval, which is "equal" to '0'::interval.
At least one thing concerns me about my code: Given an interval i, I palloc() space to calculate -i; then either return that or the original input depending on the result of a comparison. Will I leak space as a result? Should I free the value if I don't return it?
In addition to adding abs(interval) and related @ operator, I would like to update interval_smaller and interval_larger to change < and > to <= and >= respectively. This is to make the min(interval) and max(interval) aggregates return the first of multiple distinct "equal" intervals, contrary to the current behaviour:
odyssey=> select max (i) from (values ('1 month -30 days'::interval), ('-1 month 30 days'))t(i);
max
------------------
-1 mons +30 days
(1 row)
odyssey=> select min (i) from (values ('1 month -30 days'::interval), ('-1 month 30 days'))t(i);
min
------------------
-1 mons +30 days
(1 row)
odyssey=>
GREATEST and LEAST already take the first value:
odyssey=> select greatest ('1 month -30 days'::interval, '-1 month 30 days');
greatest
----------------
1 mon -30 days
(1 row)
odyssey=> select least ('1 month -30 days'::interval, '-1 month 30 days');
least
----------------
1 mon -30 days
(1 row)
odyssey=>
On Mon, 29 Mar 2021 at 21:06, Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Mar 29, 2021 at 07:15:19PM -0400, John Naylor wrote:
> Looking in the archives, I see this attempt that you can build upon:
> https://www.postgresql.org/message-id/flat/CAHE3wggpj%2Bk-zXLUdcBDRe3oahkb21pSMPDm-HzPjZxJn4vMMw%40mail.gmail.com
I see no problem with doing something more here. If you can get a
patch, please feel free to add it to the next commit fest, for
Postgres 15:
https://commitfest.postgresql.org/33/
--
Michael
Вложения
Isaac Morland <isaac.morland@gmail.com> writes:
> I've attached a patch for this. Turns out there was a comment in the source
> explaining that there is no interval_abs because it's not clear what to
> return; but I think it's clear that if i is an interval the larger of i and
> -i should be considered to be the absolute value, the same as would be done
> for any type; essentially, if the type is orderable and has a meaningful
> definition of unary minus, the definition of abs follows from those.
The problem with that blithe summary is the hidden assumption that
values that compare "equal" aren't interesting to distinguish. As
the discussion back in 2009 pointed out, this doesn't help you decide
what to do with cases like '1 month -30 days'::interval. Either answer
you might choose seems pretty arbitrary --- and we've got more than
enough arbitrariness in type interval :-(
For similar reasons, I find myself mighty suspicious of your proposal
to change how max(interval) and min(interval) work. That cannot make
things any better overall --- it will only move the undesirable results
from one set of cases to some other set. Moreover your argument for
it seems based on a false assumption, that the input values can be
expected to arrive in a particular order. So I'm inclined to think
that backwards compatibility is sufficient reason to leave that alone.
If we wanted to make some actual progress here, maybe we should
reconsider the definition of equality/ordering for interval, with
an eye to not allowing two intervals to be considered "equal" unless
they really are identical. That is, for two values that are currently
reported as "equal", apply some more-or-less-arbitrary tiebreak rule,
say by sorting on the individual fields left-to-right. This would be
very similar to type text's rule that only bitwise-equal strings are
really equal, even if strcoll() claims otherwise. I am not sure how
feasible this idea is from a compatibility standpoint, but it's
something to think about.
regards, tom lane
I wrote:
> Isaac Morland <isaac.morland@gmail.com> writes:
>> I've attached a patch for this. Turns out there was a comment in the source
>> explaining that there is no interval_abs because it's not clear what to
>> return; but I think it's clear that if i is an interval the larger of i and
>> -i should be considered to be the absolute value, the same as would be done
>> for any type; essentially, if the type is orderable and has a meaningful
>> definition of unary minus, the definition of abs follows from those.
> The problem with that blithe summary is the hidden assumption that
> values that compare "equal" aren't interesting to distinguish.
After thinking about this some more, it seems to me that it's a lot
clearer that the definition of abs(interval) is forced by our comparison
rules if you define it as
CASE WHEN x < '0'::interval THEN -x ELSE x END
In particular, this makes it clear what happens and why for values
that compare equal to zero. The thing that is bothering me about
the formulation GREATEST(x, -x) is exactly that whether you get x
or -x in such a case depends on a probably-unspecified implementation
detail inside GREATEST().
BTW, you could implement this by something along the lines of
(cf generate_series_timestamp()):
MemSet(&interval_zero, 0, sizeof(Interval));
if (interval_cmp_internal(interval, &interval_zero) < 0)
return interval_um(fcinfo);
else
PG_RETURN_INTERVAL_P(interval);
which would avoid the need to refactor interval_um().
regards, tom lane
On Sun, 26 Sept 2021 at 13:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
> Isaac Morland <isaac.morland@gmail.com> writes:
>> I've attached a patch for this. Turns out there was a comment in the source
>> explaining that there is no interval_abs because it's not clear what to
>> return; but I think it's clear that if i is an interval the larger of i and
>> -i should be considered to be the absolute value, the same as would be done
>> for any type; essentially, if the type is orderable and has a meaningful
>> definition of unary minus, the definition of abs follows from those.
> The problem with that blithe summary is the hidden assumption that
> values that compare "equal" aren't interesting to distinguish.
After thinking about this some more, it seems to me that it's a lot
clearer that the definition of abs(interval) is forced by our comparison
rules if you define it as
CASE WHEN x < '0'::interval THEN -x ELSE x END
In particular, this makes it clear what happens and why for values
that compare equal to zero. The thing that is bothering me about
the formulation GREATEST(x, -x) is exactly that whether you get x
or -x in such a case depends on a probably-unspecified implementation
detail inside GREATEST().
Thanks very much for continuing to think about this. It really reinforces my impression that this community takes seriously input and suggestions, even when it takes some thought to work out how (or whether) to proceed with a proposed change.
So I think I will prepare a revised patch that uses this formulation; and if I still have any suggestions that aren't directly related to adding abs(interval) I will split them off into a separate discussion.
BTW, you could implement this by something along the lines of
(cf generate_series_timestamp()):
MemSet(&interval_zero, 0, sizeof(Interval));
if (interval_cmp_internal(interval, &interval_zero) < 0)
return interval_um(fcinfo);
else
PG_RETURN_INTERVAL_P(interval);
which would avoid the need to refactor interval_um().
regards, tom lane
> On 26 Sep 2021, at 19:58, Isaac Morland <isaac.morland@gmail.com> wrote: > So I think I will prepare a revised patch that uses this formulation; and if I still have any suggestions that aren't directlyrelated to adding abs(interval) I will split them off into a separate discussion. This CF entry is marked Waiting on Author, have you had the chance to prepare an updated version of this patch? -- Daniel Gustafsson https://vmware.com/
On Thu, 4 Nov 2021 at 08:08, Daniel Gustafsson <daniel@yesql.se> wrote:
> On 26 Sep 2021, at 19:58, Isaac Morland <isaac.morland@gmail.com> wrote:
> So I think I will prepare a revised patch that uses this formulation; and if I still have any suggestions that aren't directly related to adding abs(interval) I will split them off into a separate discussion.
This CF entry is marked Waiting on Author, have you had the chance to prepare
an updated version of this patch?
Not yet, but thanks for the reminder. I will try to get this done on the weekend.
On Fri, Nov 05, 2021 at 12:06:05AM -0400, Isaac Morland wrote: > Not yet, but thanks for the reminder. I will try to get this done on the > weekend. Seeing no updates, this has been switched to returned with feedback in the CF app. -- Michael