305968 - [PostgreSQL] Comparrison between TIMESTAMP and TIMESTAMP(0) prevents bugmail delivery.

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Wade Klaver]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.8) Gecko/20050519 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.8) Gecko/20050519 Firefox/1.0.4

longdescs.bug_when is a TIMESTAMP(0).  $newcomments is determined with a call:
my ($newcomments, $anyprivate) = GetLongDescriptionAsText($id, $start, $end);
In this case $end is simply now(), which is by default TIMESTAMP.  This results
in GetLongDescriptionAsText() selecting no rows when the bug is modified in the
top half of the second as the check "<= '$end'" fails. (i.e. 10:25:47 <=
10:25:46.76645).  now() simply needs to be cast to TIMESTAMP(0).

The following patch appears to solve the problem:
[bugzilla@archeron Bugzilla]#cvs diff BugMail.pm
Index: BugMail.pm
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v
retrieving revision 1.39.4.2
diff -r1.39.4.2 BugMail.pm
150c150,151
<     SendSQL("SELECT " . join(',', @::log_columns) . ", lastdiffed, now() " .
---
>     SendSQL("SELECT " . join(',', @::log_columns) . ", lastdiffed, " .
>             "now()::timestamp(0) " .


Reproducible: Sometimes

Steps to Reproduce:
1. Random... simply make many reassignments to a bug for example.
2.
3.

Actual Results:  
see details.

Expected Results:  
see details.

Comment 1

[Max Kanat-Alexander]

Yes, this is a standard sort of problem. We need to find some way to override
the NOW() function in PostgreSQL so that it returns a TIMESTAMP(0).

I'm fairly sure that this is possible; we may have to create a schema (Pg
schema, not Bugzilla::DB::Schema) to do it.

Comment 2

[Emmanuel Seyman]

Attachment: replace NOW() with LOCALTIMESTAMP(0)

Comment 3

[Frédéric Buclin]

I think that NOW() should be replaced *everywhere* within the bz code, not only here.

Comment 4

[Emmanuel Seyman]

(In reply to comment #3)
>
> I think that NOW() should be replaced *everywhere* within the bz code, not only
> here.

Agreed.
If mkanat approves this, I'll submit a new bug and attach that patch there.

Comment 5

[Vlad Dascalu]

Comment on attachment 202081 [details] [diff] [review]
replace NOW() with LOCALTIMESTAMP(0)

http://pgsql.digipedia.pl/pgsql/functions-datetime.html#FUNCTIONS-DATETIME-CURRENT

>>  The function now() is the traditional PostgreSQL equivalent to CURRENT_TIMESTAMP. <<

>> CURRENT_TIME and CURRENT_TIMESTAMP deliver values with time zone; LOCALTIME and LOCALTIMESTAMP deliver values without time zone. <<

So practically, this patch would introduce a time offset based on the difference between the time with the timezone and the time without the timezone, causing either a "dead" period for the log entries, or an overlapping period of time which is processed twice.

Comment 6

[Emmanuel Seyman]

(In reply to comment #5)
> 
> So practically, this patch would introduce a time offset based on the
> difference between the time with the timezone and the time without the
> timezone, causing either a "dead" period for the log entries, or an overlapping
> period of time which is processed twice.

Are you sure?
On my installation, lastdiffed is of type "timestamp(0) without time zone" so
LOCALTIME would be the right thing to use.

mkanat, any input?

Comment 7

[Max Kanat-Alexander]

Comment on attachment 202081 [details] [diff] [review]
replace NOW() with LOCALTIMESTAMP(0)

It needs to be CURRENT_TIMESTAMP. That's the ANSI standard. Is LOCALTIMESTAMP in the ANSI standard anywhere?

Comment 8

[Max Kanat-Alexander]

> So practically, this patch would introduce a time offset based on the
> difference between the time with the timezone and the time without the
> timezone

  No. Bugzilla never uses timezone-stamped times in the database.

Comment 9

[Emmanuel Seyman]

Attachment: CURRENTT_TIMESTAMP, it is

Comment 10

[Vlad Dascalu]

(In reply to comment #8)
> > So practically, this patch would introduce a time offset based on the
> > difference between the time with the timezone and the time without the
> > timezone
> 
>   No. Bugzilla never uses timezone-stamped times in the database.
> 

Wrong, if Bugzilla uses NOW, then NOW delivers time-zone information.

The second patch looks correct, since CURRENT_TIMESTAMP is identical with NOW(). However, not sure if the correct approach would be to change everything in every place, or to create a Pg Schema thing so that would be easy customizable in the future for Sybase, Oracle or other DBMS with this issue.

Comment 11

[Emmanuel Seyman]

(In reply to comment #10)
>
> Wrong, if Bugzilla uses NOW, then NOW delivers time-zone information.

From the MySQL documentation:

Beginning with MySQL 4.1.3, the CURRENT_TIMESTAMP(), CURRENT_TIME(), CURRENT_DATE(), and FROM_UNIXTIME() functions return values in the connection's current time zone, which is available as the value of the time_zone system variable.

and:

CURRENT_TIMESTAMP and CURRENT_TIMESTAMP() are synonyms for NOW().

I'm unsure as to NOW()'s behaviour for versions of MySQL before 4.1.3.

> The second patch looks correct, since CURRENT_TIMESTAMP is identical with
> NOW(). However, not sure if the correct approach would be to change everything
> in every place, or to create a Pg Schema thing so that would be easy
> customizable in the future for Sybase, Oracle or other DBMS with this issue.

IMHO, SQL code should be ANSI whenever possible. If a DB doesn't understand this code, it should be given DB-specific code via its schema.

Comment 12

[Frédéric Buclin]

mkanat, could this bug be the reason why we didn't get all bugmail yesterday on b.e.c?

Comment 13

[Max Kanat-Alexander]

(In reply to comment #10)
> Wrong, if Bugzilla uses NOW, then NOW delivers time-zone information.

  The only difference is that NOW() and CURRENT_TIMESTAMP have the TZ appended to them when they are selected. LOCALTIMESTAMP doesn't have the TZ appended. They return the exact same number otherwise. Try it out on an actual PostgreSQL installation, you'll see what I mean.

  We never store the time-zone in the tables, on either MySQL or on PostgreSQL.


  By the way, eseyman: I realized that you were right, and it's LOCALTIMESTAMP that we need, not CURRENT_TIMESTAMP. My apologies.

  However, you are right that we need to chop off the milliseconds on LOCALTIMESTAMP, so I think our only choice at the moment is to create a Pg schema with its own custom NOW() function that returns a LOCALTIMESTAMP(0).

Comment 14

[Emmanuel Seyman]

(In reply to comment #13)
>
>   However, you are right that we need to chop off the milliseconds on
> LOCALTIMESTAMP, so I think our only choice at the moment is to create a Pg
> schema with its own custom NOW() function that returns a LOCALTIMESTAMP(0).

Is this a short or long term resolution ?
If long term, this means that we'll have to make custom schemas for all DBs that we want to support which don't implement NOW().

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> ...we'll have to make custom schemas for all DBs
> that we want to support which don't implement NOW().

  Do you know of any DBs that don't support NOW()? I can't think of any.

Comment 16

[Emmanuel Seyman]

(In reply to comment #15)
> 
>   Do you know of any DBs that don't support NOW()? I can't think of any.

According to Lance Larsh on bug 296668#c8, he uses a private function to emulate NOW() (Oracle uses SYSDATE instead).

Comment 17

[Emmanuel Seyman]

Attachment: Again, with LOCALTIMESTAMP(0)

Resubmitting my first patch for this bug.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 205979 [details] [diff] [review]
Again, with LOCALTIMESTAMP(0)

LOCALTIMESTAMP(0) is *not* a valid MySQL construct, as far as I know.

Comment 19

[Frédéric Buclin]

Comment on attachment 205979 [details] [diff] [review]
Again, with LOCALTIMESTAMP(0)

Wrong! LOCALTIMESTAMP and LOCALTIMESTAMP() have been added in MySQL 4.0.6, and we support MySQL 4.0.14, so this use is fine.

Comment 20

[Frédéric Buclin]

Oh.. and the doc says they are both synonyms for NOW().

Comment 21

[Max Kanat-Alexander]

Comment on attachment 205979 [details] [diff] [review]
Again, with LOCALTIMESTAMP(0)

OK. But we're not changing this everywhere in the Bugzilla code. This is a workaround for now.

I'm nearly certain that eventually we'll have to have a NOW() function in the Pg schema, anyhow.

Comment 22

[Frédéric Buclin]

LOCALTIMESTAMP has been introduced in MySQL 4.0.6. As Bugzilla 2.20 still supports MySQL 3.23.41, we cannot take it for 2.20.

Comment 23

[Frédéric Buclin]

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.59; previous revision: 1.58
done

Comment 24

[Frédéric Buclin]

*** Bug 325452 has been marked as a duplicate of this bug. ***