Closed Bug 257315 Opened 20 years ago Closed 19 years ago

type of delta_ts in bugs table should not be timestamp

Categories

(Bugzilla :: Bugzilla-General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: Tomas.Kopal, Assigned: Tomas.Kopal)

References

Details

Attachments

(1 file, 9 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040820 Debian/1.7.2-4
Build Identifier: 

As discussed in bug 156834, the timestamp SQL type should be replaced by
datetime. This bug is against delta_ts column in bugs table. Patch will follow.

Reproducible: Always
Steps to Reproduce:
Blocks: 156834
Attachment #157318 - Flags: review?
>> +# 2004-08-29 - Tomas.Kopal@altap.cz, bug ???

I assume you want to put the bug number in the patch :-)
Assignee: justdave → Tomas.Kopal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 2.20
Attachment #157318 - Flags: review? → review-
Attached patch Fixed bug number in comment (obsolete) — Splinter Review
Ooops :-). Thanks.
Attachment #157318 - Attachment is obsolete: true
Attachment #157369 - Flags: review?
Attached patch V3 of the patch (obsolete) — Splinter Review
Minor change in checksetup.pl to avoid annoying message on every run, even when
the type is not actually changed.
Attachment #157369 - Attachment is obsolete: true
Attachment #157369 - Flags: review?
Attachment #157558 - Flags: review?
Attachment #157558 - Flags: review? → review?(vladd)
Attachment #157558 - Flags: review?(vladd) → review?
Attachment #157558 - Flags: review?(kiko)
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Attachment #157558 - Flags: review?(vladd)
Attachment #157558 - Flags: review?(kiko)
Attachment #157558 - Flags: review?
Comment on attachment 157558 [details] [diff] [review]
V3 of the patch

Sorry to take so long to get here.

This is now bitrotten and I get several failures when trying to apply to the
tip. However they aren't many and I expect they are pretty easy to fix.

I reviewed it and it looks good so probably it will be good to go in the next
step from my point of view.

Since you'll repost it, the identation issues that I noticed are:

+if (($fielddef = GetFieldDef("bugs", "delta_ts")) &&
+    $fielddef->[1] =~ /^timestamp/) {
+ChangeFieldType ('bugs', 'delta_ts', 'DATETIME NOT NULL');
+}
+


-		  . " WHERE bug_id = $bugid");
+		   . "$diffed WHERE bug_id = $bugid");


-		  . " WHERE bug_id = $bugid");
+		   . "$diffed WHERE bug_id = $bugid");

(in fact this sounds like duplication, maybe we could avoid that, of course in
another bug if it's complicated)
Attachment #157558 - Flags: review?(vladd) → review-
Actually ignore the last 2 identation issues, it seems they were "broken" in a
way and you were trying to fix those :-) The first one still applies though.
Attached patch V4 of the patch (obsolete) — Splinter Review
Un-bitrotted, checked, polished, glazed, gold plated, simply ready for next
review round :-).
I have also added updates to two files in the contrib directory, don't know
what is the policy - should I try to keep them up-to-date or just ignore? If b)
is correct, then just ignore the two parts of the patch :-).
Attachment #157558 - Attachment is obsolete: true
Attachment #171615 - Flags: review?(vladd)
Status: NEW → ASSIGNED
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Attachment #171615 - Flags: review?(vladd) → review+
Flags: approval?
Comment on attachment 171615 [details] [diff] [review]
V4 of the patch

>RCS file: /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v
>-$sql .= "$::userid, now(), ";
>+$sql .= "$::userid, now(), now(), ";

>+        SendSQL("UPDATE bugs SET delta_ts = NOW()," .

I think we should avoid using now() here. $timestamp has already been defined
and I would prefer something like (assuming you put the definition of
$timestamp higher in the code):

$sql .="$::userid, $timestamp, $timestamp";
UPDATE bugs SET delta_ts = $timestamp

vladd and I disagree on this point, that the "cleanup" process should be part
of another bug. But as you are doing stuff on it, it seems fair to me to do it
as well.


>RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v

At line 1100, we still have: SendSQL("UPDATE bugs SET delta_ts=NOW() WHERE
bug_id=$i"); This line is not part of your code, but as you are considering
delta_ts, my opinion is that it should also be converted to $timestamp.


If justdave says it's ok like this, then remove my r- and open a bug on that.
Attachment #171615 - Flags: review-
Attached patch V5 of the patch (obsolete) — Splinter Review
Well, here is the proposed cleanup incorporated into the patch. The changes are
not so intrusive, so I think it's ok to add them here... Feel free to ignore it
and check in V4 of you do not agree.
Attachment #171615 - Attachment is obsolete: true
Attachment #171733 - Flags: review?
Attachment #171733 - Flags: review? → review+
Comment on attachment 171733 [details] [diff] [review]
V5 of the patch

>Index: importxml.pl
>===================================================================
>+  push (@query, "delta_ts");
>+  if ( (defined $bug_fields{'delta_ts'}) && ($bug_fields{'delta_ts'}) ){
>+      push (@values, SqlQuote($bug_fields{$field}));
>+  }


Sorry, I have to deny review again. ./runtests.pl is complaining about
importxml.pl, line 351 (test 001). The reason is that $field is undefined. It
should be $bug_fields{'delta_ts'}.

Nit: I would also be *very* happy if you could change globals.pl, line 141 from
delta_ts = now() to delta_ts = $timestamp as this variable is already defined
and used in the insert just above it. :)

With these two comments fixed, I'm ok for a r+.


Note, also to Vlad: Did you check that all existing "UPDATE bugs" in
checksetup.pl are correct? This is the single file which I did not investigate
seriously.
Attachment #171733 - Flags: review-
Attached patch V6 of the patch (obsolete) — Splinter Review
(In reply to comment #10)
> (From update of attachment 171733 [details] [diff] [review] [edit])
> >Index: importxml.pl
> >===================================================================
> >+  push (@query, "delta_ts");
> >+  if ( (defined $bug_fields{'delta_ts'}) && ($bug_fields{'delta_ts'}) ){
> >+	  push (@values, SqlQuote($bug_fields{$field}));
> >+  }
> 
> 
> Sorry, I have to deny review again. ./runtests.pl is complaining about
> importxml.pl, line 351 (test 001). The reason is that $field is undefined. It

> should be $bug_fields{'delta_ts'}.

Good one, thanks. Fixed.

> 
> Nit: I would also be *very* happy if you could change globals.pl, line 141
from
> delta_ts = now() to delta_ts = $timestamp as this variable is already defined

> and used in the insert just above it. :)
> 
> With these two comments fixed, I'm ok for a r+.
> 

If it comes to make other people happy, I am always willing to do my best ;-) -
fixed.
While verifying that the callers are really passing usable value here, I have
also added the timestamp parameter to the call on marking a bug diplicate and
confirming a bug by popular vote to make all the DB changes with the same
timestamp. I understand it's getting a bit further from the subject of this
bug, but it's a minor change and obviously correct. Feel free to disagree and
remove it from the patch :-).

> 
> Note, also to Vlad: Did you check that all existing "UPDATE bugs" in
> checksetup.pl are correct? This is the single file which I did not
investigate
> seriously.
> 

In the first versions of the patch, I added the column change to the top of the
DB updates in checksetup.pl, and changed all subsequent usage. But that seems
to be a bit too intrusive and error prone, so in the recent version, the column
change is done as a last change in checksetup.pl and no other queries are
modified. This ensures that if someone have older database, it will be updated
first (using still timestamp type) and then the type is changed. No changes are
needed for new DBs, so it should always work. Does it make sense?
Attachment #171733 - Attachment is obsolete: true
Attachment #172147 - Flags: review?
Attached patch V7 of the patch (obsolete) — Splinter Review
Bugger, wrong file, sorry :-).
Attachment #172147 - Attachment is obsolete: true
Attachment #172158 - Flags: review?
Attachment #172147 - Flags: review?
Comment on attachment 172158 [details] [diff] [review]
V7 of the patch

OK, I'm fully satisfied with this patch! :)

About checksetup.pl, I have applied your patch, used Bugzilla a bit and nothing
bad happened. *If* there is any problem about that, we will open a new bug
about checksetup.pl only.

r=LpSolit
Attachment #172158 - Flags: review? → review+
Comment on attachment 172158 [details] [diff] [review]
V7 of the patch

I've taken a look at the checksetup.pl part and it looks ok to me.
Attachment #172158 - Flags: review+
Flags: approval? → approval+
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.224; previous revision: 1.223
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.65; previous revision: 1.64
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.331; previous revision: 1.330
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.27; previous revision: 1.26
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.64; previous revision: 1.63
done
Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.26; previous revision: 1.25
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.288; previous revision: 1.287
done
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.38; previous revision: 1.37
done
Checking in move.pl;
/cvsroot/mozilla/webtools/bugzilla/move.pl,v  <--  move.pl
new revision: 1.29; previous revision: 1.28
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.97; previous revision: 1.96
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.228; previous revision: 1.227
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.74; previous revision: 1.73
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.23; previous revision: 1.22
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.23; previous revision: 1.22
done
Checking in contrib/bug_email.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v  <--  bug_email.pl
new revision: 1.21; previous revision: 1.20
done
Checking in contrib/jb2bz.py;
/cvsroot/mozilla/webtools/bugzilla/contrib/jb2bz.py,v  <--  jb2bz.py
new revision: 1.2; previous revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Blocks: 280265
This has been backed out.  It critically breaks things in its current form (see
bug 280265)
Status: RESOLVED → REOPENED
Flags: approval+
Resolution: FIXED → ---
No longer blocks: 280265
*** Bug 280265 has been marked as a duplicate of this bug. ***
Comment on attachment 172158 [details] [diff] [review]
V7 of the patch

see previous comment
Attachment #172158 - Flags: review-
Attached patch V3 Fix for the previous patch (obsolete) — Splinter Review
New version of the fix. Adds quotes and SqlQuote calls as neccesary, does not
remove any SqlQuote calls.
I still think that we should remove the SqlQuote calls where they are not
neccessary, as they are part of the deprecated Bugzilla::DB functionality we
want to get rid of, but I agree this shoud be done in a separate bug and not
here.
Note that this is not complete patch for delta_ts, it needs to be applied on
top of the previous one (backed out). I think it will make review much easier
than if I merge them.
Attachment #172859 - Flags: review?(mkanat)
Is it not easier to change $timestamp to something like:

# get current time
SendSQL("SELECT NOW()");
my $timestamp = SqlQuote(FetchOneColumn());

or:
# get current time
SendSQL("SELECT NOW()");
my $timestamp = FetchOneColumn();
my $sql_timestamp = SqlQuote($timestamp);

So that you don't need to write '$timestamp' or SqlQuote($timestamp) everywhere?
Comment on attachment 172859 [details] [diff] [review]
V3 Fix for the previous patch

By inspection, it looks good. You catched all the missing quotes.

I missed this '$timestamp' stuff (with quotes) when reviewing previous patches;
sorry for that.

I need to test it a bit before granting review. But my previous comment still
applies: having a sql_timestamp instead of writing '$now' or 'timestamp' may be
better, IMO.
Attachment #172859 - Flags: review?(LpSolit)
(In reply to comment #20)
> Is it not easier to change $timestamp to something like:
> 
> # get current time
> SendSQL("SELECT NOW()");
> my $timestamp = SqlQuote(FetchOneColumn());
> 
> or:
> # get current time
> SendSQL("SELECT NOW()");
> my $timestamp = FetchOneColumn();
> my $sql_timestamp = SqlQuote($timestamp);
> 
> So that you don't need to write '$timestamp' or SqlQuote($timestamp) everywhere?

Well, maybe. As I said, I would prefer not to use SqlQuote as it's deprecated
function which should go away eventually, and it's not neccessary for values
returned directly from DB anyway. But if you insist, I will fix it :-).
Tomas, adding quotes in a single place is better. This prevent us from
forgetting them! ;)

I discussed that point with mkanat in IRC and he agrees with me.
(In reply to comment #23)
> Tomas, adding quotes in a single place is better. This prevent us from
> forgetting them! ;)
> 
> I discussed that point with mkanat in IRC and he agrees with me.
> 

Point taken, do you want them as plain quotes, or SqlQuote? (or with fries? ;-) ).
(In reply to comment #24)
> Point taken, do you want them as plain quotes, or SqlQuote?

Personally, I prefer SqlQuote(). Or maybe Bugzilla->dbh->quote() only?
Attached patch V4 Fix for the previous patch (obsolete) — Splinter Review
Simple quotes changed to SqlQuote. Again, applies over the previous patch.
Attachment #172859 - Attachment is obsolete: true
Attachment #172919 - Flags: review?
Attachment #172859 - Flags: review?(mkanat)
Attachment #172859 - Flags: review?(LpSolit)
Comment on attachment 172919 [details] [diff] [review]
V4 Fix for the previous patch

r=LpSolit

Now please merge this patch with the previous valid one, so that we only have
one patch to check in. But, please, do not change anything so that I can easily
forward my r+. ;)
Attachment #172919 - Flags: review? → review+
Just a merge of V7 patch and V4 fix, nothing else.
Attachment #172158 - Attachment is obsolete: true
Attachment #172919 - Attachment is obsolete: true
Attachment #173149 - Flags: review?
Comment on attachment 173149 [details] [diff] [review]
V8, merged and ready to go

You know, I hate to do this so late in the review cycle, but I noticed
something bad last time the patch was checked-in:

The checksetup code is in the *wrong place*. It needs to go above the comment
about "if you change the --TABLE-- definition", higher up in the file. Just
search for the word "--TABLE--".
Attachment #173149 - Flags: review? → review-
(In reply to comment #29)
> The checksetup code is in the *wrong place*. It needs to go above the comment
> about "if you change the --TABLE-- definition", higher up in the file.

Correct! I realized that now. But anyway, a *lot* of code is outside these
bounds and maybe having it fixed in another patch would be fine too, so that we
can fix them all at once... and have this one finally fixed. Do you agree with
my suggestion?
Comment on attachment 173149 [details] [diff] [review]
V8, merged and ready to go

OK, agreed with Tomas. This looks like a correct merge of the two patches, and
I'm willing to certify that.
Attachment #173149 - Flags: review- → review+
Comment on attachment 173149 [details] [diff] [review]
V8, merged and ready to go

I just want to make sure once again that there is no problem with this patch...
just to be sure. ;)
Attachment #173149 - Flags: review?(LpSolit)
(In reply to comment #29)
> (From update of attachment 173149 [details] [diff] [review] [edit])
> You know, I hate to do this so late in the review cycle, but I noticed
> something bad last time the patch was checked-in:
> 
> The checksetup code is in the *wrong place*. It needs to go above the comment
> about "if you change the --TABLE-- definition", higher up in the file. Just
> search for the word "--TABLE--".
> 

Well, there probably is some rationale behind this rule, but I fail to see it. I
see mostly adding of fields above the mentioned line, but modifications after
(see e.g. "# 2004/02/15 - Summaries shouldn't be null - see bug 220232"). Even
my previous patch for timestamp type has the change AFTER this line (search for
"# 2004-08-29 - Tomas.Kopal@altap.cz, bug 257303").
My reason to put it at the end is that I don't have to modify every subsequent
piece of code touching bugs table to treat delta_ts in a different way - if it
is old DB which needs upgrading, all upgrades are done first on the old
timestamp type, and then the type is converted. If it does not need updating,
nothing happens. The only limitation is that all further changes to the bugs
table in the checksetup.pl must be placed after this change, i.e. in
chronological order (which is what I tought is happening).
So, can someone explain me where I am wrong?
(In reply to comment #30)
> (In reply to comment #29)
> > The checksetup code is in the *wrong place*. It needs to go above the comment
> > about "if you change the --TABLE-- definition", higher up in the file.
> 
> Correct! I realized that now. But anyway, a *lot* of code is outside these
> bounds and maybe having it fixed in another patch would be fine too, so that we
> can fix them all at once... and have this one finally fixed. Do you agree with
> my suggestion?
> 

Ok, I think understand now, the other changes below that line are wrong too :-).
Then I would agree to fix it in another patch, as we should move all changes as
a block, to keep them in the same order as they are now. Otherwise we can cause
more trouble than we'll fix...
(In reply to comment #30)
> Correct! I realized that now. But anyway, a *lot* of code is outside these
> bounds and maybe having it fixed in another patch would be fine too, so that we
> can fix them all at once... and have this one finally fixed. Do you agree with
> my suggestion?
> 

I just opened bug 280856 about that comment.
Comment on attachment 173149 [details] [diff] [review]
V8, merged and ready to go

r=LpSolit

I have tested this patch again on a fresh installation and no problem has been
detected. And the new patch is indeed the merge of the two previous ones, as
expected.

Thanks Tomas! :)
Attachment #173149 - Flags: review?(LpSolit) → review+
Flags: approval?
Flags: approval? → approval+
Minor bitrot on the CGI.pl section due to checkin of bug 276838 just an hour or 
so ago. The fix was intuitive (remove $::unconfirmedstate, add UNCONFIRMED) so 
I made the change before checking it in.

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.227; previous revision: 1.226
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.67; previous revision: 1.66
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.336; previous revision: 1.335
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.29; previous revision: 1.28
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.67; previous revision: 1.66
done
Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.28; previous revision: 1.27
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.298; previous revision: 1.297
done
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.40; previous revision: 1.39
done
Checking in move.pl;
/cvsroot/mozilla/webtools/bugzilla/move.pl,v  <--  move.pl
new revision: 1.31; previous revision: 1.30
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.100; previous revision: 1.99
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.233; previous revision: 1.232
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.77; previous revision: 1.76
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.26; previous revision: 1.25
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.30; previous revision: 1.29
done
Checking in contrib/bug_email.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v  <--  bug_email.pl
new revision: 1.23; previous revision: 1.22
done
Checking in contrib/jb2bz.py;
/cvsroot/mozilla/webtools/bugzilla/contrib/jb2bz.py,v  <--  jb2bz.py
new revision: 1.4; previous revision: 1.3
done
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: