280500 - Replace "DATE_FORMAT()" with Bugzilla::DB function call

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Tomas Kopal]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050110 Firefox/1.0 (Debian package 1.0+dfsg.1-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050110 Firefox/1.0 (Debian package 1.0+dfsg.1-2)

"DATE_FORMAT" is MySQL specific and should be replaced with DB
agnostic call to function in the DB compatibility layer.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Tomas Kopal]

Attachment: V1

Patch replacing DATE_FORMAT SQL with DB method call. I have also modified the
regular expression used for date/time formating in Util.pm to accept more
formats, as Postgres is returning also fractions of seconds.

Comment 2

[Max Kanat-Alexander]

I'm torn about this.

Bugzilla really should never see those fractions of seconds. But if we use
NOW(), they will be put in the database.

The problem is that some code might use a perl timestamp, which would only have
seconds, and some code might use a DB timestamp, which has fractions of seconds.

I ran into this problem quite a bit in my local PostgreSQL install, and had to
patch it.

The fact that we can have inconsistent timestamps is a problem.

Is there any way, in the schema or in something that we can do in Bugzilla or
DBI to have PostgreSQL *not* return fractions of seconds?

Comment 3

[Max Kanat-Alexander]

Oh, and note that we would also have to not *insert* fractions of seconds. That
is, dates would have to somehow globally become seconds-only, even with NOW().

Otherwise we need to do some major investigative work to make sure that we
always use DB-time for comparisons, and never perl time.

Comment 4

[Max Kanat-Alexander]

By the way, none of that is a comment on the patch itself, it's just a question
that I need to have answered before I grant review+.

My only other note is that Bugzilla should always return something consistent
for the weekday stuff, in Bugzilla::DB, instead of the way that you had to patch
whine.pl. (If that's not possible, feel free to explain why.)

Comment 5

[Tomas Kopal]

(In reply to comment #4)
> My only other note is that Bugzilla should always return something consistent
> for the weekday stuff, in Bugzilla::DB, instead of the way that you had to patch
> whine.pl. (If that's not possible, feel free to explain why.)

Trouble is that MySQL has weekdays numbering starting with 0 (0-6, Sunday=0),
and Postgres has numbering starting with 1 (1-7, Sunday=1).

We can probably come up with a solution converting one or the other to some
defined range (although I am not sure how, as you can't simply add -1 to the
format string :-) ), but I think textual representation is less ambiguous, and
ATM it's currently used on only one place, which is quite easy to change.

If you have some simple solution on your mind, it will be great to make the
function more flexible :-).

Comment 6

[Tomas Kopal]

(In reply to comment #2)
> Is there any way, in the schema or in something that we can do in Bugzilla or
> DBI to have PostgreSQL *not* return fractions of seconds?

I haven't found any way to switch Pg to not to use fractions globally. So the
only way I see ATM is to change all places where we work with times to use
formatting (either using TO_CHAR() or DATE_TRUNC(), both Pg specific). That's a
big change to bz.
Other option is to leave DB to use what it wants, and ignore what we don't need
(that's what I was trying to do with the Util.pm patch). I agree that we need to
make sure that when the time passes the boundary from DB to perl (DB usually
accepts pretty much anything, so the other direction should be ok), it has to do
it using sql_date_format method from DB or the format_time from Util, never
directly. It seems to be much simpler change than the first option though.
The fix to Util.pm seems to do the trick for my installation, I haven't changed
anything else and it seems to work. But I will do a closer inspection of bz code
if you agree that this is the way to go.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)

  Right, but here's the problem: sometimes we will need to do date comparisons
*in SQL*. Those date comparisons can't be different than the date/time
comparisons that we do in perl. I can't even imagine the host of bugs that we
would experience.

  We can't limit ourselves to only doing date comparisons in SQL, or only doing
date comparisons in perl. That would be too limiting, and many of the scripts
that we have now wouldn't work.

  We need to handle this in some fashion or another, very definitely.

Comment 8

[Tomas Kopal]

(In reply to comment #7)
> (In reply to comment #6)
> 
>   Right, but here's the problem: sometimes we will need to do date comparisons
> *in SQL*. Those date comparisons can't be different than the date/time
> comparisons that we do in perl. I can't even imagine the host of bugs that we
> would experience.
> 

Hmmm, I still can't see any part of this doom scenario. If you are doing
comparisons in SQL, then you are comparing data coming either from SQL (i.e.
correct format for given DB) or from perl (then SQL converts the perl format to
sql format and then it does comparison). Both cases works correctly.

The only problem I can see here is if:
- you are doing the comparison as strings. This is a bad thing to do and I think
it's ok to get incorrect result.
- you rely on the timestamps to be the same, but they are not due to some
previous removal of the fraction. This is IMHO another bad think and we should
not do it (as you should never compare floating number for equality, you should
not rely on times to be equal).

I agree that it's possible that one of these cases (especially the second one)
may be a problem in the current code, and it may be hard to find all of the
occurences. Still, do you know of any better way how to deal with this problem?

One solution comes to my mind and that is swithing from native SQL dates to
something general. Someone suggested previously on some other Pg bug to switch
to strings (problems with sorting) or to integers. But that's definitely a much
bigger change...

BTW, the change I did does nothing on MySQL, so it can break only Postgres. I
would say that we should try it as it is (after some code inspections as I
already suggested) and if something does not work on Pg, we can deal with it on
case by case basis.

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> Hmmm, I still can't see any part of this doom scenario. If you are doing
> comparisons in SQL, then you are comparing data coming either from SQL (i.e.
> correct format for given DB) or from perl (then SQL converts the perl format 
> to sql format and then it does comparison). Both cases works correctly.

  Actually, they don't. For example, in perl 1:23:45 == 1:23:45.6789. However,
in SQL those are different times. One is greater than the other. Take for
example the comparisons of lastdiffed that we do in BugMail.pm or
GetLongDescriptionAsText (this is where the bug showed up in the Red Hat bugzilla).

> - you rely on the timestamps to be the same, but they are not due to some
> previous removal of the fraction. This is IMHO another bad think and we should
> not do it (as you should never compare floating number for equality, you 
> should  not rely on times to be equal).

  In MySQL, times are never floating-point numbers. So it was fine to compare them.

  Even if we're not talking about equality, we could be talking about
*comparison at all*. For example 1:23:45.6789 > 1:23:45 is TRUE in PostgreSQL,
and FALSE in MySQL. 

    Sometimes we insert a perl timestamp into a MySQL date column. So it has no
fractions of seconds in PostgreSQL. And then we compare it with a SQL time.

    Any time we do a date comparison of a date, we rely on the behavior of MySQL
that I've listed above, that such a statement be FALSE.

> I agree that it's possible that one of these cases (especially the second one)
> may be a problem in the current code, and it may be hard to find all of the
> occurences. Still, do you know of any better way how to deal with this 
> problem?

  I think that we should stop using perl time in all of Bugzilla, and only use
SQL time. That's the only good solution that I can think of, and I think it
would cover it.

> One solution comes to my mind and that is swithing from native SQL dates to
> something general. Someone suggested previously on some other Pg bug to switch
> to strings (problems with sorting) or to integers. But that's definitely a 
> much bigger change...

  Yeah, we decided against it because we would lose indexes on date comparisons
in the SQL, which could kill performance.

> BTW, the change I did does nothing on MySQL, so it can break only Postgres. I
> would say that we should try it as it is (after some code inspections as I
> already suggested) and if something does not work on Pg, we can deal with it 
> on case by case basis.

  OK. Could you file another bug for this investigation, though, that we have to
do about dates?

Comment 10

[Tomas Kopal]

(In reply to comment #9)
>   I think that we should stop using perl time in all of Bugzilla, and only use
> SQL time. That's the only good solution that I can think of, and I think it
> would cover it.

That sounds like an interesting idea. I will take a look at the code how big
change that would be...

> 
> > One solution comes to my mind and that is swithing from native SQL dates to
> > something general. Someone suggested previously on some other Pg bug to switch
> > to strings (problems with sorting) or to integers. But that's definitely a 
> > much bigger change...
> 
>   Yeah, we decided against it because we would lose indexes on date comparisons
> in the SQL, which could kill performance.

I am afraid I don't uderstand. If you switch to integers (e.g. seconds since
epoch, unix format), you should be able to do comparisons as you wish, you
should be able to index etc. The only difference is that you need different
formatting function when you want to print it out. What am I missing?

>   OK. Could you file another bug for this investigation, though, that we have to
> do about dates?

Done, see bug 283076.

Comment 11

[Max Kanat-Alexander]

Tomas, update this to remove the date-formatting stuff, and I'll r=mkanat.

Comment 12

[Tomas Kopal]

Attachment: V2

Removed change of date formatting in Util.pm.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 175415 [details] [diff] [review]
V2

r=mkanat by inspection

Comment 14

[Max Kanat-Alexander]

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.228; previous revision: 1.227
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.72; previous revision: 1.71
done
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.283; previous revision: 1.282
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.305; previous revision: 1.304
done
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.19; previous revision: 1.18
done
Checking in whine.pl;
/cvsroot/mozilla/webtools/bugzilla/whine.pl,v  <--  whine.pl
new revision: 1.9; previous revision: 1.8
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.19; previous revision: 1.18
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.57; previous revision: 1.56
done

Comment 15

[Max Kanat-Alexander]

There was a trailing comma in Bug.pm line 152 after I checked-in this patch
(pointed out by glob).

I checked-in a fix for it so that show_bug won't be broken anymore. I just
removed the trailing comma.

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