491954 - Views should select exact columns

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Shawn Wilsher :sdwilsh]

Our views should select exact columns so we don't kill our ability to downgrade between versions.  This *must* be fixed before we ship 1.9.1 or we'll stick ourselves into a nasty nasty hole.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch

well, really we should avoid "SELECT *" everywhere we UNION.
Just to be safe for future changes i've removed all SELECT * FROM real_table, and replaced with COLUMNS macros.
Plus moved indexes to their own file.

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 376363 [details] [diff] [review]
patch

>+++ b/toolkit/components/places/src/nsAnnotationService.cpp
>@@ -127,8 +128,7 @@ nsAnnotationService::Init()
> 
>   // mDBGetAnnotation
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>-      "SELECT * "
>-      "FROM moz_annos "
>+      "SELECT " MOZ_ANNOS_COLUMNS " FROM moz_annos "
This isn't needed since we don't use temporary tables here.  Please don't add complexity like this unless we have to.

>@@ -136,8 +136,7 @@ nsAnnotationService::Init()
> 
>   // mDBGetItemAnnotation
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>-      "SELECT * "
>-      "FROM moz_items_annos "
>+      "SELECT " MOZ_ITEMS_ANNOS_COLUMNS " FROM moz_items_annos "
ditto

>+++ b/toolkit/components/places/src/nsPlacesIndexes.h
>@@ -0,0 +1,165 @@
>+/* vim: sw=2 ts=2 sts=2 expandtab
Can I suggest coping what storage uses please?  See http://mxr.mozilla.org/mozilla-central/source/storage/style.txt

r=sdwilsh with changes

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 4

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/e6c08971cd7e

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 376685 [details] [diff] [review]
patch v1.1

this should either block or have approval, we need this change to allow any future schema upgrade.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking as per comment 0

Comment 7

[Marco Bonardo [:mak]]

thanks
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f3ca8b75d84e

Comment 8

[Marco Bonardo [:mak]]

argh, in the change i exactly forgot to change the view creation, me bad!
additional patch coming.

Comment 9

[Marco Bonardo [:mak]]

Attachment: additional patch v1.0

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 376825 [details] [diff] [review]
additional patch v1.0

ugh - I recall thinking that too but I was going to add it at the end of the review comments, and apparently I forgot.  *sigh*

r=sdwilsh

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ca08e548a105

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/308ae27ef9d8

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marking verified fixed based on checkins into trunk and 1.9.1 branch. There is no way for me to check this manually.