Обсуждение: Is there a better way to do this?

Hi all

I want to implement something like the following:

CREATE OR REPLACE FUNCTION AddDays
        (TIMESTAMP WITHOUT TIME ZONE
        , INT)
RETURNS TIMESTAMP WITHOUT TIME ZONE AS '
DECLARE
        time ALIAS FOR $1;
        days ALIAS FOR $2;
BEGIN
        RETURN time+days*24*3600*''1 second''::INTERVAL;
END;
' LANGUAGE 'plpgsql';

Basically the function takes two parameters, and add the second one (as
days) onto the first one (as timestamp without  timezone)
I don't really like this implementation. Is there a more concise way to
do this?

Thanks

Wei

On Tue, Aug 28, 2007 at 04:59:46PM -0400, Wei Weng wrote:
> Hi all
>
> I want to implement something like the following:

Well, you could always implement it as SQL instead (untested):

CREATE OR REPLACE FUNCTION AddDays
       (TIMESTAMP WITHOUT TIME ZONE
       , INT)
RETURNS TIMESTAMP WITHOUT TIME ZONE AS '
SELECT $1+$2*24*3600*''1 second''::INTERVAL;
' LANGUAGE 'sql';

Not sure if it qualifies as 'more concise' though.

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

On Aug 28, 2007, at 15:59 , Wei Weng wrote:

> I don't really like this implementation. Is there a more concise
> way to do this?

create or replace function add_days(timestamp, int)
returns timestamp language sql as $body$
select $1 + $2 * interval '1 day'
$body$;

Note that interval '1 day' is not equal to interval '24 hours'. '1
day' can be 23 or 25 hours across daylight saving time boundaries.

If you mean 24 hours (which you're getting with your 24 * 3600 *
interval '2 second'), you could do

create or replace function add_24_hour_intervals(timestamp, int)
returns timestamp language sql as $body$
select $1 + $2 * interval '24 hours';
$body$;

Do you *really* want timestamp without time zone? It's not like
you're gaining any performance or decreasing storage requirements.
Unless you have a reason for *not* using with time zone, I'd
recommend using timestamp with time zone.

Hope this helps.

Michael Glaesemann
grzm seespotcode net

Wei Weng escreveu:
> Hi all
>
> I want to implement something like the following:
>
> CREATE OR REPLACE FUNCTION AddDays
>        (TIMESTAMP WITHOUT TIME ZONE
>        , INT)
> RETURNS TIMESTAMP WITHOUT TIME ZONE AS '
> DECLARE
>        time ALIAS FOR $1;
>        days ALIAS FOR $2;
> BEGIN
>        RETURN time+days*24*3600*''1 second''::INTERVAL;
> END;
> ' LANGUAGE 'plpgsql';
>
> Basically the function takes two parameters, and add the second one (as
> days) onto the first one (as timestamp without  timezone)
> I don't really like this implementation. Is there a more concise way to
> do this?
>


CREATE OR REPLACE FUNCTION AddDays
        (time TIMESTAMP WITHOUT TIME ZONE
        , days INT)
RETURNS TIMESTAMP WITHOUT TIME ZONE AS $$
BEGIN
        RETURN time + days*'1 day'::INTERVAL;
END;
$$ LANGUAGE 'plpgsql';


Osvaldo

On 8/28/07, Wei Weng <wweng@kencast.com> wrote:
> Is there a more concise way to do this?

CREATE OR REPLACE FUNCTION
ADDDAYS (TIMESTAMP WITHOUT TIME ZONE, INT)
RETURNS TIMESTAMP WITHOUT TIME ZONE AS '
SELECT $1+($2 * ''1 DAY''::INTERVAL)
' LANGUAGE SQL;

Wei Weng wrote:
> I want to implement something like the following:
> CREATE OR REPLACE FUNCTION AddDays

You don't know how many seconds are in a day, so just add the days using
SQL.

    RETURN time + (days || ' days')::INTERVAL;

You don't even need to make that a function, just do that you your SQL
directly.

-- Dante

On Aug 28, 2007, at 16:51 , Michael Glaesemann wrote:

> If you mean 24 hours (which you're getting with your 24 * 3600 *
> interval '2 second'), you could do

Or, 24 * 3600 * interval '1 second', rather


Michael Glaesemann
grzm seespotcode net

On Aug 28, 2007, at 16:55 , D. Dante Lorenso wrote:

>     RETURN time + (days || ' days')::INTERVAL;

It's bad practice to concatenate like this. Use time + days *
interval '1 day' and be done with it.


Michael Glaesemann
grzm seespotcode net

Michael Glaesemann wrote:
>
> On Aug 28, 2007, at 16:55 , D. Dante Lorenso wrote:
>>     RETURN time + (days || ' days')::INTERVAL;
> It's bad practice to concatenate like this. Use time + days * interval
> '1 day' and be done with it.

Why?  Is this functionality expected to break in the future or has
unexpected side effects?  Is it less clear or less efficient?  Who
declared it bad practice and where can I read that documentation?

-- Dante

On Aug 28, 2007, at 17:22 , D. Dante Lorenso wrote:

> Michael Glaesemann wrote:
>> On Aug 28, 2007, at 16:55 , D. Dante Lorenso wrote:
>>>     RETURN time + (days || ' days')::INTERVAL;
>> It's bad practice to concatenate like this. Use time + days *
>> interval '1 day' and be done with it.
>
> Why?  Is this functionality expected to break in the future or has
> unexpected side effects?  Is it less clear or less efficient?  Who
> declared it bad practice and where can I read that documentation?

It's generally bad practice to interpolate unnecessarily. You're
right, in this case you're probably safe from this particular case
ever changing. I personally find it less clear (though clarity is
often in the eye of the beholder). time +  * interval '1 day' is to
me a clearer expression of what you're doing: add this multiple of
days to the time.

(days || ' days')::interval says "Make a string using this value (oh,
it's an int? we need to cast it to text) and this string: the result
just happens to match the proper input format for an interval, which
is fortunate because now we're casting the string to interval". Okay,
there's a little editorializing thrown in, but that's what the
concatenation says to me.

The concatenation is probably less efficient: you're casting an int
to text and then the text to interval with the concatenation you're
using. I don't know how that compares in terms of cycles to the int *
interval math, but efficiency isn't really the reason I would avoid it.

I'm sure others could provide more cogent explanations, but those are
my initial thoughts.

Michael Glaesemann
grzm seespotcode net

On Aug 28, 2007, at 17:46 , Michael Glaesemann wrote:

> I'm sure others could provide more cogent explanations, but those
> are my initial thoughts.

Thinking about this a little bit more: pushing interpolation/
concatenation to the furthest extreme you get to using eval-like
construct, which is generally not considered something you want to
do. Not quite that far you get SQL-injection (to bring the point a
little closer to home). Now, granted you don't need to worry about
these types of things in the specific example you provided, but these
are related to the more general "it's bad practice" statement.

Michael Glaesemann
grzm seespotcode net

Michael Glaesemann wrote:
> On Aug 28, 2007, at 17:22 , D. Dante Lorenso wrote:
>> Michael Glaesemann wrote:
>>> On Aug 28, 2007, at 16:55 , D. Dante Lorenso wrote:
>>>>     RETURN time + (days || ' days')::INTERVAL;
>>> It's bad practice to concatenate like this. Use time + days *
>>> interval '1 day' and be done with it.
>>
>> Why?  Is this functionality expected to break in the future or has
>> unexpected side effects?  Is it less clear or less efficient?  Who
>> declared it bad practice and where can I read that documentation?
>
> It's generally bad practice to interpolate unnecessarily. You're right,
> in this case you're probably safe from this particular case ever
> changing. I personally find it less clear (though clarity is often in
> the eye of the beholder). time +  * interval '1 day' is to me a clearer
> expression of what you're doing: add this multiple of days to the time.

Something in my just doesn't want to trust that:

    30 * interval '1 day' == interval '30 days'

Intervals are magical things unlike int and text.  Doing multiplication
on a magical thing is scary, but waiting until the end before applying
the magic just *feels* safer.

I do like your syntax, though.  There are less parentheses.  Maybe I can
warm up to it ;-)

-- Dante

Michael Glaesemann <grzm@seespotcode.net> writes:
> Note that interval '1 day' is not equal to interval '24 hours'. '1
> day' can be 23 or 25 hours across daylight saving time boundaries.

When you are adding to timestamp without time zone, they *are*
interchangeable, since no daylight-savings arithmetic is involved.
Michael's right for the case of adding to timestamp with time zone,
though.

> Do you *really* want timestamp without time zone?

I agree with that comment --- you should at least think hard
about what you are trying to represent.  The "date" type is also
worth considering if time-of-day isn't so important to you.

            regards, tom lane

On Aug 28, 2007, at 19:30 , Tom Lane wrote:

> Michael Glaesemann <grzm@seespotcode.net> writes:
>> Note that interval '1 day' is not equal to interval '24 hours'. '1
>> day' can be 23 or 25 hours across daylight saving time boundaries.
>
> When you are adding to timestamp without time zone, they *are*
> interchangeable, since no daylight-savings arithmetic is involved.
> Michael's right for the case of adding to timestamp with time zone,
> though.

Ah, right. Thanks for pointing that out.

Michael Glaesemann
grzm seespotcode net

Michael Glaesemann <grzm@seespotcode.net> writes:
> The concatenation is probably less efficient: you're casting an int
> to text and then the text to interval with the concatenation you're
> using. I don't know how that compares in terms of cycles to the int *
> interval math, but efficiency isn't really the reason I would avoid it.

It's a *lot* less efficient: number times interval-constant is basically
three floating-point multiplies, whereas as you said, the other way
involves formatting the number as text, performing a string
concatenation, and then parsing the result to see whether it's legal
input for an interval and if so what its value is.  Here's a simple
experiment to compare the costs:

regression=# explain analyze select x from generate_series(1,100000) x;
                                                          QUERY PLAN
      

------------------------------------------------------------------------------------------------------------------------------
 Function Scan on generate_series x  (cost=0.00..12.50 rows=1000 width=4) (actual time=670.205..1337.351 rows=100000
loops=1)
 Total runtime: 1713.093 ms
(2 rows)

regression=# explain analyze select x * '1 day'::interval from generate_series(1,100000) x;
                                                          QUERY PLAN
      

------------------------------------------------------------------------------------------------------------------------------
 Function Scan on generate_series x  (cost=0.00..17.50 rows=1000 width=4) (actual time=664.579..1841.494 rows=100000
loops=1)
 Total runtime: 2222.444 ms
(2 rows)

regression=# explain analyze select (x || ' days')::interval from generate_series(1,100000) x;
                                                          QUERY PLAN
      

------------------------------------------------------------------------------------------------------------------------------
 Function Scan on generate_series x  (cost=0.00..25.00 rows=1000 width=4) (actual time=741.236..3015.771 rows=100000
loops=1)
 Total runtime: 3402.385 ms
(2 rows)

Subtracting off the overhead estimated by the first query, we arrive at
196k conversions/second using multiply vs 60k conversions/sec using
concatenation.  This is on a rather old and slow server, but the ratio
probably holds up on other hardware.  Allowing for a lot of noise in the
measurements (I didn't bother trying to average several measurements),
I'd say 2X to 4X slower is a good estimate.

As Michael says, the speed argument is really kinda minor compared
to the other ones, but it's real enough.

            regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/28/07 20:06, Tom Lane wrote:
[snip]
>
> As Michael says, the speed argument is really kinda minor compared
> to the other ones, but it's real enough.

Every little bit counts, though.  For example, if it's part of an
otherwise computationally-intensive operation that you're performing
on 20 million rows, you'll definitely see the difference.

- --
Ron Johnson, Jr.
Jefferson LA  USA

Give a man a fish, and he eats for a day.
Hit him with a fish, and he goes away for good!

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFG1OP4S9HxQb37XmcRAvxUAJ0TFcC06TgAsOv8aou3BC1pJpbADwCgkQyy
Ib8H44C7d8mvvWfsHbWjFVE=
=3/7d
-----END PGP SIGNATURE-----

On Tue, Aug 28, 2007 at 04:59:46PM -0400, Wei Weng wrote:
> Hi all
>
> I want to implement something like the following:
>
> CREATE OR REPLACE FUNCTION AddDays
>        (TIMESTAMP WITHOUT TIME ZONE
>        , INT)
> RETURNS TIMESTAMP WITHOUT TIME ZONE AS '
> DECLARE
>        time ALIAS FOR $1;
>        days ALIAS FOR $2;
> BEGIN
>        RETURN time+days*24*3600*''1 second''::INTERVAL;
> END;
> ' LANGUAGE 'plpgsql';

This seems like a lot of extra work.

SELECT now() + 5 * INTERVAL '1 day';

does a similar trick, and is quite clear as to what it does :)

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
phone: +1 415 235 3778        AIM: dfetter666
                              Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate