602926 - MSSQL doesn't support the NOW() SQL function

Status: ASSIGNED

(Bugzilla :: Database, enhancement)

Description

[c1541]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.11) Gecko/20101001 Firefox/3.6.11
Build Identifier: 

MSSQL doesn't support the NOW() SQL function, however it does support the CURRENT_TIMESTAMP function, as do MySQL, pgSQL & Oracle, so could possibly replace all occurences of NOW() with CURRENT_TIMESTAMP.

However, there is also the use of CURRENT_DATE in the Bugzilla code as well, which unfortunatly isn't supported by MSSQL, so that would need an alternative approach.

Perhaps a sql_date_now function (with a parameter to specify if time needs to be included) is needed.

sub sql_date_now {
    my ($self, $date_type) = @_;
    if ($date_type == "time" ) {
        return "CURRENT_TIMESTAMP";
    } else{
        return "CURRENT_DATE";
    }
}


Reproducible: Always

Comment 1

[Michael Thomas (Mockodin)]

 re comment 0
>sub sql_date_now {
>    my ($self, $date_type) = @_;
>    if ($date_type == "time" ) {
>        return "CURRENT_TIMESTAMP";
>    } else{
>        return "CURRENT_DATE";
>    }
>}

-- Part of the reason I'm building th port as SQL 2008 only)
 CAST(CURRENT_TIMESTAMP AS DATE)

Also we are doing the same evil as the Oracle port as using adjust sql to do the following:

s/NOW\(\)/GETDATE()/g

If the other DBs use CURRENT_TIMESTAMP then that would certainly the best overall option to move to.

Comment 2

[c1541]

Could also be done with two functions without parameters:

sub sql_current_date {
    return "CURRENT_DATE";
}

sub sql_current_timestamp {
    return "CURRENT_TIMESTAMP";
}

Comment 3

[c1541]

(In reply to comment #1)
> -- Part of the reason I'm building th port as SQL 2008 only)
>  CAST(CURRENT_TIMESTAMP AS DATE)

That can be done as:
CONVERT(datetime, CONVERT(char(8), getdate(), 112), 112)
(or numerous other ways) for SQL 2005 if needed.

Comment 4

[c1541]

CURRENT_TIMESTAMP docs links

mySQL: http://dev.mysql.com/doc/refman/5.0/en/date-and-time-functions.html#function_current-timestamp
pgSQL: http://www.postgresql.org/docs/8.3/static/functions-datetime.html
oracle: http://download.oracle.com/docs/cd/E14072_01/server.112/e10592/functions043.htm

Comment 5

[Frédéric Buclin]

Bug 296668 already plans to replace NOW() by something else. Looks like it should be CURRENT_TIMESTAMP rather than LOCALTIMESTAMP.

Comment 6

[c1541]

Attachment: Changes NOW() to use a function

Here's an untested patch.

Comment 7

[Frédéric Buclin]

Comment on attachment 486458 [details] [diff] [review]
Changes NOW() to use a function

>=== modified file 'Bugzilla/DB.pm'

>+sub sql_current_date {
>+    return "CURRENT_DATE";
>+}
>+
>+sub sql_current_timestamp {
>+    return "CURRENT_TIMESTAMP";
>+}

I don't get it. What's the point in replacing NOW() by CURRENT_TIMESTAMP via a method if this method is not overridden anywhere?

Comment 8

[Max Kanat-Alexander]

Comment on attachment 486458 [details] [diff] [review]
Changes NOW() to use a function

Thanks for your patch, but this is not what we want, see earlier comments.

Comment 9

[c1541]

Would a patch that just replaces NOW() with CURRENT_TIMESTAMP be better?

Comment 10

[Michael Thomas (Mockodin)]

c1541 see comment 5
bug 296658 already exists to address the underlying issue.

Comment 11

[Max Kanat-Alexander]

Well...does CURRENT_TIMESTAMP(0) work on all of our supported databases? If it does, that might be the way to go, although I'm not sure. (The (0) may be required for PostgreSQL in some situations.)

But yes, in general, replacing NOW() with CURRENT_TIMESTAMP would be a good way to go, since CURRENT_TIMESTAMP seems to be a more widely-implemented standard.

Comment 12

[c1541]

Attachment: just direct replacements without a perl function

Here's anew patch which just replaces NOW() and LOCALTIMESTAMP() with CURRENT_TIMESTAMP
Also removes the functions added for Oracle & sqlite.

Comment 13

[Frédéric Buclin]

Comment on attachment 487170 [details] [diff] [review]
just direct replacements without a perl function

>=== modified file 'Bugzilla/DB/Sqlite.pm'

>-sub _sqlite_now {
>-    my $now = DateTime->now(time_zone => Bugzilla->local_timezone);
>-    return $now->ymd . ' ' . $now->hms;
>-}

>-    $self->sqlite_create_function('now', 0, \&_sqlite_now);
>-    $self->sqlite_create_function('localtimestamp', 1, \&_sqlite_now);


Your patch looks good, but there is a small bitrot in Bugzilla/DB/Sqlite.pm due to the checkin of bug 630681. Could you update your patch to fix this small problem? :)
Also, CURRENT_TIMESTAMP in SQLite returns the current time in UTC, not using the server timezone, see http://www.sqlite.org/lang_createtable.html:
"If the default value of a column is CURRENT_TIME, CURRENT_DATE or CURRENT_TIMESTAMP, then the value used in the new row is a text representation of the current UTC date and/or time."

But maybe there is a way to work around this problem in Perl itself. After all, it seems storing data in UTC is the way to go, to avoid DST problems. mkanat, would you agree to take this patch despite the timezone problem mentioned above? Only SQLite behaves this way. MySQL, PostgreSQL and Oracle takes the timezone into account. (and we shouldn't wait too long to approve such patches, because they can bitrot pretty quickly.)

Comment 14

[Max Kanat-Alexander]

Comment on attachment 487170 [details] [diff] [review]
just direct replacements without a perl function

Unless I'm mistaken, this will break PostgreSQL. We do LOCALTIMESTAMP(0) to strip milliseconds, which will break us.

Comment 15

[Max Kanat-Alexander]

Oh, and if CURRENT_TIMESTAMP is UTC always, then we definitely don't want to use it.

We have a separate bug about always using UTC in the database, but that's a much, much larger issue.

Even if we did always use UTC, we'd probably do it by setting the DB's timezone to UTC, so we'd still want a constructor that returns the local timestamp (without milliseconds).

Comment 16

[Frédéric Buclin]

(In reply to comment #14)
> Unless I'm mistaken, this will break PostgreSQL. We do LOCALTIMESTAMP(0) to
> strip milliseconds, which will break us.

In our current code, we use both LOCALTIMESTAMP(0) and NOW(). CURRENT_TIMESTAMP behaves exactly the same way as NOW() in PostgreSQL, so I don't think we will regress (we would have to check this point).


(In reply to comment #15)
> Oh, and if CURRENT_TIMESTAMP is UTC always, then we definitely don't want to
> use it.

This only happens with SQLite. MySQL, PostgreSQL and Oracle all return the time using the server timezone.

Comment 17

[Max Kanat-Alexander]

(In reply to comment #16)
> In our current code, we use both LOCALTIMESTAMP(0) and NOW().
> CURRENT_TIMESTAMP behaves exactly the same way as NOW() in PostgreSQL, so I
> don't think we will regress (we would have to check this point).

  Ah, actually it's NOW() that caused the problems in Pg. We use it in some places because it's fine that it has milliseconds.

> > Oh, and if CURRENT_TIMESTAMP is UTC always, then we definitely don't want to
> > use it.
> 
> This only happens with SQLite. MySQL, PostgreSQL and Oracle all return the
> time using the server timezone.

  Ugh. Does the SQL spec say anything about what timezone this is supposed to be in?

Comment 18

[Frédéric Buclin]

(In reply to comment #17)
>   Ugh. Does the SQL spec say anything about what timezone this is supposed
> to be in?

The SQL-92 specification says in section 6.8:

"The <datetime value function>s CURRENT_DATE, CURRENT_TIME, and
 CURRENT_TIMESTAMP respectively return the current date, current
 time, and current timestamp; the time and timestamp values are
 returned with time zone displacement equal to the current time
 zone displacement of the SQL-session."

http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt

Comment 19

[Jasmin Sehic]

Considering this is already handled by patch in bug 483437 by using the statement modifier s/NOW\(\)/GETDATE()/g I suggest to mkanat that the bz-mssql blocking be removed for the time being as it doesn't prevent the MS-SQL support from functioning correctly.

Comment 20

[Jasmin Sehic]

I meant to say:

... already handled by patch in bug 487437 ....

Comment 21

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 22

[Jasmin Sehic]

As I detailed in Comment 19 there is probably no need for this change as proposed change in bug 487437 will handle this will no impact as it will be isolated to MSSQL specifics.