745533 - database add index audit_log_class_at_time_idx for audit_log

Status: RESOLVED FIXED

(Bugzilla :: Database, defect, P1)

Description

[Frank Becker]

follow up of Bug#740536:
(In reply to Frédéric Buclin from comment #25)
> (In reply to Max Kanat-Alexander from comment #24)
> >  BTW, you're going to need an index on the class field now. Otherwise your
> > DB servers are going to be in a world of pain when people start to use this
> > API.
> 
> dkl or Frank: please file a bug for the DB index.

Comment 1

[Frank Becker]

Attachment: patch V1

Comment 2

[Max Kanat-Alexander]

Comment on attachment 615138 [details] [diff] [review]
patch V1

Review of attachment 615138 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/DB/Schema.pm
@@ +506,5 @@
>              added     => {TYPE => 'MEDIUMTEXT'},
>              at_time   => {TYPE => 'DATETIME', NOTNULL => 1},
>          ],
> +        INDEXES => [
> +                    audit_log_class_at_time_idx => ['class', 'at_time'],

Indexes need to be named by their first field only. So this would be audit_log_class_idx. Also, why do you need at_time in this index? That's going to make it much larger.

Comment 3

[Frank Becker]

Attachment: patch V2

(In reply to Max Kanat-Alexander from comment #2)
> Comment on attachment 615138 [details] [diff] [review]
> patch V1
> 
> Review of attachment 615138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Bugzilla/DB/Schema.pm
> @@ +506,5 @@
> >              added     => {TYPE => 'MEDIUMTEXT'},
> >              at_time   => {TYPE => 'DATETIME', NOTNULL => 1},
> >          ],
> > +        INDEXES => [
> > +                    audit_log_class_at_time_idx => ['class', 'at_time'],
> 
> Indexes need to be named by their first field only. So this would be
> audit_log_class_idx. Also, why do you need at_time in this index? That's
> going to make it much larger.

I add the at_time to the index so that we can get the max only by access to the index. Otherwise you end with on entry in the index for every class and this entry has a long rid list.

With the two columns we have 8 bytes more per entry but the database engine can pick the first/last entry that matches the class value.

BTW maybe we need to add an descending to make sure that the max is the first entry.

Comment 4

[Frédéric Buclin]

Increasing severity as it blocks 4.4rc1.

Comment 5

[David Lawrence [:dkl]]

Comment on attachment 615438 [details] [diff] [review]
patch V2

Review of attachment 615438 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=dkl

::: Bugzilla/DB/Schema.pm
@@ +506,5 @@
>              added     => {TYPE => 'MEDIUMTEXT'},
>              at_time   => {TYPE => 'DATETIME', NOTNULL => 1},
>          ],
> +        INDEXES => [
> +                    audit_log_class_idx => ['class', 'at_time'],

nit: fix indentation on checkin

Comment 6

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #5)
> Looks good. r=dkl

I hope it's more than just a "looks good". :) We cannot afford checksetup.pl to fail or corrupt the DB when upgrading the schema. Make sure this patch has been tested before checking in.

Comment 7

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #6)
> (In reply to David Lawrence [:dkl] from comment #5)
> > Looks good. r=dkl
> 
> I hope it's more than just a "looks good". :) We cannot afford checksetup.pl
> to fail or corrupt the DB when upgrading the schema. Make sure this patch
> has been tested before checking in.

Yes, Admittedly I should have been more specific than 'looks good' ;) But I do normally test a patch before r+ and in this case I did the following verifications:

1. Install new db from scratch and observed the index was created properly.
2. Upgraded a previous test database that did not yet have the index and observed the new index was created properly. 
3. Ran several queries using the class column and 'explain' to see that the index was indeed being used.

Everything tested fine. I assume the patch author has tested it locally as well. 

dkl

Comment 8

[Frédéric Buclin]

So I will let you commit this patch, as you reviewed it. ;)

Comment 9

[David Lawrence [:dkl]]

trunk:
Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/Install/DB.pm
modified Bugzilla/DB/Schema.pm
Committed revision 8260.

Comment 10

[Frank Becker]

(In reply to David Lawrence [:dkl] from comment #7)
> (In reply to Frédéric Buclin from comment #6)
> > (In reply to David Lawrence [:dkl] from comment #5)
> > > Looks good. r=dkl
> > 
> > I hope it's more than just a "looks good". :) We cannot afford checksetup.pl
> > to fail or corrupt the DB when upgrading the schema. Make sure this patch
> > has been tested before checking in.
> 
> Yes, Admittedly I should have been more specific than 'looks good' ;) But I
> do normally test a patch before r+ and in this case I did the following
> verifications:
> 
> 1. Install new db from scratch and observed the index was created properly.
> 2. Upgraded a previous test database that did not yet have the index and
> observed the new index was created properly. 
> 3. Ran several queries using the class column and 'explain' to see that the
> index was indeed being used.
> 
> Everything tested fine. I assume the patch author has tested it locally as
> well. 

Yes, I did the some tests (1. and 2.), but did not use explain 

> 
> dkl