301141 - [PostgreSQL] New charts throw an error about FROM_UNIXTIME

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Frédéric Buclin]

Create a CSV file using new charts on Pg:

      undef error - DBD::Pg::st execute failed: ERROR:  function
from_unixtime(integer) does not exist [for Statement "SELECT
TO_CHAR(series_date, 'J')::int - TO_CHAR(FROM_UNIXTIME(961138800), 'J')::int,
series_value FROM series_data WHERE series_id = ? AND series_date >=
FROM_UNIXTIME(961138800) AND series_date <= FROM_UNIXTIME(1120287600)"]
at Bugzilla/Chart.pm line 259
	Bugzilla::Chart::readData('Bugzilla::Chart=HASH(0x9ae44b0)') called at
Bugzilla/Chart.pm line 200
	Bugzilla::Chart::data('Bugzilla::Chart=HASH(0x9ae44b0)') called at
data/template/template/en/default/reports/chart.csv.tmpl line 19
	eval {...} called at data/template/template/en/default/reports/chart.csv.tmpl
line 19
	eval {...} called at data/template/template/en/default/reports/chart.csv.tmpl
line 16
	Template::Provider::__ANON__('Template::Context=HASH(0x9aadda0)') called at
/usr/lib/perl5/site_perl/5.8.5/i386-linux-thread-multi/Template/Document.pm line 144
	eval {...} called at
/usr/lib/perl5/site_perl/5.8.5/i386-linux-thread-multi/Template/Document.pm line 142
	Template::Document::process('Template::Document=HASH(0x9bc7854)',
'Template::Context=HASH(0x9aadda0)') called at
/usr/lib/perl5/site_perl/5.8.5/i386-linux-thread-multi/Template/Context.pm line 315
	eval {...} called at
/usr/lib/perl5/site_perl/5.8.5/i386-linux-thread-multi/Template/Context.pm line 295
	Template::Context::process('Template::Context=HASH(0x9aadda0)',
'Template::Document=HASH(0x9bc7854)') called at
/usr/lib/perl5/site_perl/5.8.5/i386-linux-thread-multi/Template/Service.pm line 90
	eval {...} called at
/usr/lib/perl5/site_perl/5.8.5/i386-linux-thread-multi/Template/Service.pm line 88
	Template::Service::process('Template::Service=HASH(0x9a84b90)',
'reports/chart.csv.tmpl', 'HASH(0x9a0f1cc)') called at
/usr/lib/perl5/site_perl/5.8.5/i386-linux-thread-multi/Template.pm line 59
	Template::process('Bugzilla::Template=HASH(0x9a42ff0)',
'reports/chart.csv.tmpl', 'HASH(0x9a0f1cc)') called at
/var/www/html/bugzilla-tip-pg/chart.cgi line 276
	main::plot() called at /var/www/html/bugzilla-tip-pg/chart.cgi line 119


I can reproduce the problem on both my local installation using PostgreSQL 8.0.1
and on landfill (Pg 7.x IIRC). WFM using MySQL 4.1.

Comment 1

[Max Kanat-Alexander]

Yeah, I think we can just use sql_date_format or format_time to fix that.

Comment 2

[Shin-ichi Nagamura]

Attachment: sql_from_epoch

This bug is because postgresql doesn't understand FROM_UNIXTIME() function.
Then I make a abstract method for same function.

This method given a time from epoch, then return a time format string.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 191352 [details] [diff] [review]
sql_from_epoch

I think this code can be restructured to not need sql_from_epoch. I don't
really want to create another sql_ function when it's only going to be used
here.

Comment 4

[Shin-ichi Nagamura]

OK, but I don't know same function for unix epoch time in mysql and postgresql.
I have a idea and it is use strftime() in perl code. 

For example.

use POSIX 'strftime' ;

#$dbh->sql_to_days("FROM_UNIXTIME($datefrom)") #original code
$dbh->sql_to_days( strftime( "%Y-%m-%d %H:%M:%S", $datefrom ))

Comment 5

[Frédéric Buclin]

Attachment: patch, v1

Comment 6

[Frédéric Buclin]

Comment on attachment 201871 [details] [diff] [review]
patch, v1

karl, could you test this patch? It's about new charts.

Comment 7

[A. Karl Kornel]

Comment on attachment 201871 [details] [diff] [review]
patch, v1

(In reply to comment #6)
> karl, could you test this patch? It's about new charts.

Looks OK to me.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 201871 [details] [diff] [review]
patch, v1

I really don't like that '::date' argument.

The times when it needs to be cast to a ::date is when you pass in a string-literal date instead of a field name.

So, instead of adding a parameter, just add "::date" whenever the argument is a string literal.

Comment 9

[Frédéric Buclin]

(In reply to comment #8)
> So, instead of adding a parameter, just add "::date" whenever the argument is a
> string literal.

Huh?? How do I do that???

Comment 10

[Max Kanat-Alexander]

A string literal will always be passed in like q{'1970-01-01'}. Thus, it will always have single-quotes around it. Check for the single-quotes at the edges of the string.

Comment 11

[Frédéric Buclin]

(In reply to comment #10)
> A string literal will always be passed in like q{'1970-01-01'}. Thus, it will
> always have single-quotes around it. Check for the single-quotes at the edges
> of the string.


Hum... this doesn't work, because $dbh->sql_to_days() gets a '?', not a string. So you have no idea what will be passed to the placeholder. Am I missing something?
Moreover, I don't think that quotes are part of the string.

Comment 12

[Frédéric Buclin]

Attachment: patch, v2

I think this is definitely the right fix, i.e. cast the date in all cases. But we have to remove

AddFDef("(" . $dbh->sql_to_days('NOW()') . " - " .
              $dbh->sql_to_days('bugs.delta_ts') . ")",
        "Days since bug changed", 0);

from checksetup.pl, see bug 302599. This is the single place which causes error.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 202438 [details] [diff] [review]
patch, v2

(In reply to comment #12)
> I think this is definitely the right fix, i.e. cast the date in all cases. But
> we have to remove

  We don't have to remove it, we just have to make the field longer. The error is that the contents are too long.

  But yes, otherwise I like this solution much better.

Comment 14

[Max Kanat-Alexander]

The listed bug is not actually the blocker. It's true, Dave has suggested that we switch to using a special "name" with a separate "definition" for all fielddefs, but we're not going to do that during the 2.22 freeze. (It would be a large change.)

What we need to do instead is enlarge the name varchar before we run the AddFDef functions, which can be done in this bug.

Comment 15

[Frédéric Buclin]

(In reply to comment #14)
> What we need to do instead is enlarge the name varchar before we run the
> AddFDef functions, which can be done in this bug.

This is another bug (which should block this one), and the fix you are suggesting is a workaround only. But if we haven't any better solution in the short term, let's do it. :( Suggesting 128 characters instead of 64?

Comment 16

[Max Kanat-Alexander]

You're right, it would be another bug. Yeah, 128 sounds fine to me.

You probably shouldn't request approval on this one until we've actually checked-in the other one.

Comment 17

[Frédéric Buclin]

Hum... we still have a problem. My DB already has the field in question and fixing $dbh->sql_to_days() introduce a second one:

bugs_pg=> select name, length(name) as longi from fielddefs order by longi desc;
                                   name                                    | longi
---------------------------------------------------------------------------+-------
 (TO_CHAR(NOW()::date, 'J')::int - TO_CHAR(bugs.delta_ts::date, 'J')::int) |    73
 (TO_CHAR(NOW(), 'J')::int - TO_CHAR(bugs.delta_ts, 'J')::int)             |    61
 attachments.description                                                   |    23

If we have to rename the old field, we could as well rename it correctly, i.e. a real name, not a SQL fragment.

Comment 18

[Max Kanat-Alexander]

(In reply to comment #17)
> Hum... we still have a problem. My DB already has the field in question and
> fixing $dbh->sql_to_days() introduce a second one:

  Ah, yeah, I knew about that. :-) I'd forgotten until you mentioned it just now, though. You're going to have to special-case this particular AddFDef, for now, and check for the exact old text if it's Pg. Fun, I know.

Comment 19

[Frédéric Buclin]

(In reply to comment #18)
>   Ah, yeah, I knew about that. :-) I'd forgotten until you mentioned it just
> now, though. You're going to have to special-case this particular AddFDef, for
> now, and check for the exact old text if it's Pg. Fun, I know.

I'm on it. Saved searches have to be fixed too. I will post my patch in bug 302599. If everything goes well, my patch should be ready in less than an hour.

Comment 20

[Max Kanat-Alexander]

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

Comment 21

[Dave Miller [:justdave]]

ok, the status whiteboard says "hold approval until blocker is checked-in" but the only thing in the dependency fields is a metabug that depends on this, doesn't block it.  What am I missing?  Is it bug 302599?  That's mentioned in passing, but nobody actually says it blocks this one anywhere...

Comment 22

[Frédéric Buclin]

The blocker has a+ on it, this one can land too.

Comment 23

[Frédéric Buclin]

Checking in Bugzilla/Chart.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Chart.pm,v  <--  Chart.pm
new revision: 1.9; previous revision: 1.8
done
Checking in Bugzilla/DB/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v  <--  Pg.pm
new revision: 1.17; previous revision: 1.16
done