Closed Bug 491954 Opened 11 years ago Closed 11 years ago

Views should select exact columns

Categories

(Toolkit :: Places, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: sdwilsh, Assigned: mak)

References

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 1 obsolete file)

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.
Flags: blocking1.9.1?
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #376363 - Flags: review?(sdwilsh)
Severity: normal → critical
OS: Mac OS X → All
Hardware: x86 → All
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
Attachment #376363 - Flags: review?(sdwilsh) → review+
Attached patch patch v1.1Splinter Review
Attachment #376363 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/e6c08971cd7e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #376685 - Flags: approval1.9.1?
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.
Blocks: 488966
Blocking as per comment 0
Flags: blocking1.9.1? → blocking1.9.1+
Attachment #376685 - Flags: approval1.9.1?
Depends on: 492379
argh, in the change i exactly forgot to change the view creation, me bad!
additional patch coming.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #376825 - Flags: review?(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
Attachment #376825 - Flags: review?(sdwilsh) → review+
http://hg.mozilla.org/mozilla-central/rev/ca08e548a105
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 493538
Marking verified fixed based on checkins into trunk and 1.9.1 branch. There is no way for me to check this manually.
Status: RESOLVED → VERIFIED
Blocks: 450516
You need to log in before you can comment on or make changes to this bug.