1224718 - Consider adding UNIQUE constraint to the History.GUID database field

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

iOS sets this to unique [1]: should we do the same?

It's technically more correct (GUID!) but if we've broken that constraint in the past, the migration will fail, or places where we break that constraint might throw SQLiteExceptions. 

[1]: https://github.com/mozilla/firefox-ios/blob/e077523cc0581c0b3939350243158a03a721e8a3/Storage/SQL/BrowserTable.swift#L143

Comment 1

[Richard Newman [:rnewman]]

Reasons to do so:

* Validation: you want an insert to fail because it violates the uniqueness constraint. Look for places in the code that insert into history. Do they always do update-or-insert or are otherwise guaranteed uniqueness?

* Indexing: uniqueness constraints come with an index. (Indeed, if you add one after the fact you do so by CREATE UNIQUE INDEX.)

Presumably we already have an index on GUID, else syncing will be slow, so the only reason to add a constraint is to catch bugs.

Adding a unique index when duplicate values exist will fail.

Comment 2

[Varun Joshi (:vjoshi)]

Attachment: addUniqueConstraintToHistory.patch

Added a UNIQUE constraint to History.GUID . The migration went smoothly on my test device.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8690526 [details] [diff] [review]
addUniqueConstraintToHistory.patch

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

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +1030,5 @@
>          db.execSQL("ALTER TABLE tmp_tabs RENAME TO " + TABLE_TABS);
>          createTabsTableIndices(db, TABLE_TABS);
>          didCreateTabsTable =true;
>      }
> +    

nit: We try to avoid adding excess whitespace – can you remove the spaces on this blank line?

@@ +1033,5 @@
>      }
> +    
> +    private void upgradeDatabaseFrom25to26(SQLiteDatabase db) {
> +        // This version adds the UNIQUE constraint to History.GUID
> +        migrateHistoryTable(db);

Re-using a method from a previous migration seems fragile but looking at the code, it looks alright.

Did you consider creating a unique index like http://stackoverflow.com/a/10071366 ? It's seems better to me to do this migration in place but I'm not sure the trade-offs here – Richard?

@@ +1115,5 @@
>  
>                  case 25:
>                      upgradeDatabaseFrom24to25(db);
>                      break;
> +                    

nit: white space

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8690526 [details] [diff] [review]
addUniqueConstraintToHistory.patch

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

For future reference: this is an extraordinarily expensive way of adding a unique index! Just do this:

  CREATE UNIQUE INDEX foo_index ON table (foo)

The only reason to take a longer-winded approach is if you have smart ideas about error recovery.


However, there is *already* a unique index:

        db.execSQL("CREATE UNIQUE INDEX history_guid_index ON " + TABLE_HISTORY + '('
                + History.GUID + ')');

That line was last touched in a refactor in September 2014, so (at least!) every profile created in the last 18 months or so has a unique index on GUID.

That means this patch just recreates the exact same table and copies all the same data over.

I think this bug is INVALID. Michael?

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It looks like Richard is correct and this bug is invalid – sorry about that varunj.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Let me know if you want me to find another bug for you to work on.

Comment 7

[Varun Joshi (:vjoshi)]

(In reply to Richard Newman [:rnewman] from comment #4)
> Comment on attachment 8690526 [details] [diff] [review]
> addUniqueConstraintToHistory.patch
> 
> Review of attachment 8690526 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For future reference: this is an extraordinarily expensive way of adding a
> unique index! Just do this:
> 
>   CREATE UNIQUE INDEX foo_index ON table (foo)
> 
> The only reason to take a longer-winded approach is if you have smart ideas
> about error recovery.
> 

Adding a unique index is much better, I don't know how it slipped my mind. I'll keep this in mind next time. Thanks!

(In reply to Michael Comella (:mcomella) from comment #6)
> Let me know if you want me to find another bug for you to work on.

Sure! I'd love to work on another bug.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

bug 1128675 looks pretty similar & cool! If that doesn't float your boat, let me know!