Deleting a bug doesn't remove related data from the bugs_fulltext table

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Administration
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Ed Goose, Assigned: Ed Goose)

Tracking

3.1.4
Bugzilla 3.2
Bug Flags:
approval +
approval3.2 +
blocking3.2 +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; .NET CLR 1.0.3705; InfoPath.1; .NET CLR 1.1.4322; MS-RTC LM 8; .NET CLR 2.0.50727)
Build Identifier: 3.1.4

If you delete a bug from the bugs table, sanitycheck will throw up errors for most tables that the bug_id no longer exists for.

This is not the case for bugs_fulltext.

Reproducible: Always

Steps to Reproduce:
1. Delete a bug (in a way you shouldn't... like directly using SQL)
2. Run the sanity check, errors will be picked up in tables.
3. Query bugs_fulltext for this bug id, and a record will be still found.
Actual Results:  
Sanity check should throw up an error, and delete the bug.

Expected Results:  
Sanity check did not throw up an error.
(Assignee)

Comment 1

10 years ago
Created attachment 323843 [details] [diff] [review]
Patch for sanity check v1

Created using diff, not CVS.
(Assignee)

Updated

10 years ago
Attachment #323843 - Flags: review?(LpSolit)
(Assignee)

Updated

10 years ago
Version: unspecified → 3.1.4

Comment 2

10 years ago
(In reply to comment #0)
> 1. Delete a bug (in a way you shouldn't... like directly using SQL)

How did you manage to do that? AFAIK, foreign keys enforce the deletion of entries in the bugs_fulltext table when a bug is deleted, due to:

    bugs_fulltext => {
        FIELDS => [
            bug_id     => {TYPE => 'INT3', NOTNULL => 1, PRIMARYKEY => 1,
                           REFERENCES => {TABLE  => 'bugs',
                                          COLUMN => 'bug_id',
                                          DELETE => 'CASCADE'}},

ON DELETE CASCADE should remove obsolete entries, even if you delete the entry from the bugs table manually using SQL commands. Max?

Comment 3

10 years ago
Yeah, you would have had to explicitly tell MySQL/PgSQL to ignore keys when deleting the row for this to happen (or you somehow hacked your system and have MyISAM tables when you shouldn't).
Status: UNCONFIRMED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → INVALID

Comment 4

10 years ago
Comment on attachment 323843 [details] [diff] [review]
Patch for sanity check v1

The DB handles this, it doesn't need to be in sanitycheck.
Attachment #323843 - Flags: review?(LpSolit) → review-

Comment 5

10 years ago
It's rather WONTFIX than INVALID. sanitycheck.cgi indeed doesn't do this check, but that's on purpose.
Resolution: INVALID → WONTFIX
(Assignee)

Comment 6

10 years ago
(In reply to comment #3)
> Yeah, you would have had to explicitly tell MySQL/PgSQL to ignore keys when
> deleting the row for this to happen (or you somehow hacked your system and have
> MyISAM tables when you shouldn't).

All our tables are Innodb, except the bugs_fulltext one, which is a MyISAM table. Is this correct? If it is, I can definitely delete from bugs and it definitely doesn't cascade.
It not, how do I change it?

Comment 7

10 years ago
Oh, right, of course. :-) bugs_fulltext is MyISAM.
Status: RESOLVED → UNCONFIRMED
Resolution: WONTFIX → ---

Updated

10 years ago
Attachment #323843 - Flags: review- → review?(LpSolit)

Updated

10 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 8

10 years ago
No worries :)
Just to check... is there anywhere else where this cascading is assumed to take care of this table but it's been forgotten? For example (I still need to check this) but I don't believe the importxml.pl script is correctly making the bugs_fulltext rows for each bug.

Comment 9

10 years ago
Comment on attachment 323843 [details] [diff] [review]
Patch for sanity check v1

>+++ ./bugzilla/.cvsignore	2007-10-19 08:58:48.000000000 +0100

Please remove .cvsignore from your patch.

Your patch works fine and catches and deletes obsolete entries in bugs_fulltext correctly. But you don't need to hack the DB manually to reproduce the bug. All you have to do is to delete a bug by deleting a component. Bugzilla::Bug->remove_from_db() indeed doesn't remove entries in bugs_fulltext, which is a bug. Could you please update your patch to include this fix in remove_from_db()?
Attachment #323843 - Flags: review?(LpSolit) → review+

Comment 10

10 years ago
Marking as blocking 3.2 as remove_from_db() leaves the DB in an inconsistent state, per my previous comment.
Assignee: administration → ed.goose
Depends on: 399370
Flags: blocking3.2+
Target Milestone: --- → Bugzilla 3.2
(Assignee)

Comment 11

10 years ago
Will do asap. This may be as late as Wednesday (but no later) though.

Comment 12

10 years ago
(In reply to comment #11)
> Will do asap. This may be as late as Wednesday (but no later) though.

That's fine. Thanks.
Status: NEW → ASSIGNED
(Assignee)

Comment 13

10 years ago
Created attachment 324024 [details] [diff] [review]
Patch for Bug.pm v1

Created using diff not CVS.

Ignore my comment about it not being available until Wednesday!
Attachment #324024 - Flags: review?(LpSolit)
(Assignee)

Comment 14

10 years ago
Created attachment 324026 [details] [diff] [review]
Patch v2 for sanity check AND bug.pm

Combines the two patches, and ignores .cvsignore.

Created using diff not cvs.
Attachment #323843 - Attachment is obsolete: true
Attachment #324024 - Attachment is obsolete: true
Attachment #324026 - Flags: review?(LpSolit)
Attachment #324024 - Flags: review?(LpSolit)

Comment 15

10 years ago
Comment on attachment 324026 [details] [diff] [review]
Patch v2 for sanity check AND bug.pm

Works fine. Thanks! r=LpSolit
Attachment #324026 - Flags: review?(LpSolit) → review+

Updated

10 years ago
Severity: minor → normal
Flags: approval3.2+
Flags: approval+

Comment 16

10 years ago
OK, to be more exact, this patch works fine, but the deletion inside the bugs_fulltext table must happen after bz_commit_transaction() in case something went wrong. I will attach the patch I'm going to check in.

Comment 17

10 years ago
Created attachment 324030 [details] [diff] [review]
patch, v2.1

The patch I'm going to check in.
Attachment #324026 - Attachment is obsolete: true
Attachment #324030 - Flags: review+

Comment 18

10 years ago
tip:

Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.141; previous revision: 1.140
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.242; previous revision: 1.241
done


3.1.4:

Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.140.2.1; previous revision: 1.140
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.241.2.1; previous revision: 1.241
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Summary: Sanitycheck does not check the full text table: bugs_fulltext → Deleting a bug doesn't remove related data from the bugs_fulltext table
You need to log in before you can comment on or make changes to this bug.