Closed
Bug 491954
Opened 16 years ago
Closed 16 years ago
Views should select exact columns
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: sdwilsh, Assigned: mak)
References
Details
(Keywords: verified1.9.1)
Attachments
(2 files, 1 obsolete file)
28.35 KB,
patch
|
Details | Diff | Splinter Review | |
2.28 KB,
patch
|
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•16 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Severity: normal → critical
OS: Mac OS X → All
Hardware: x86 → All
Reporter | ||
Comment 2•16 years ago
|
||
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+
Assignee | ||
Comment 3•16 years ago
|
||
Attachment #376363 -
Attachment is obsolete: true
Assignee | ||
Comment 4•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Updated•16 years ago
|
Attachment #376685 -
Flags: approval1.9.1?
Assignee | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #376685 -
Flags: approval1.9.1?
Assignee | ||
Comment 7•16 years ago
|
||
Keywords: fixed1.9.1
Assignee | ||
Comment 8•16 years ago
|
||
argh, in the change i exactly forgot to change the view creation, me bad!
additional patch coming.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•16 years ago
|
||
Attachment #376825 -
Flags: review?(sdwilsh)
Reporter | ||
Comment 10•16 years ago
|
||
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+
Assignee | ||
Comment 11•16 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•16 years ago
|
||
Comment 13•16 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•