Last Comment Bug 634144 - Calling possible_duplicates throws an error about invalid SQL on PostgreSQL 8.4
: Calling possible_duplicates throws an error about invalid SQL on PostgreSQL 8.4
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Database (show other bugs)
: 4.0
: All All
: -- normal (vote)
: Bugzilla 4.0
Assigned To: sam morris
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-14 17:24 PST by sam morris
Modified: 2011-03-01 05:44 PST (History)
1 user (show)
mkanat: approval+
mkanat: approval4.0+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
workaround (440 bytes, patch)
2011-02-14 17:31 PST, sam morris
mkanat: review-
Details | Diff | Review
modifying sql_fulltext_search instead (1.70 KB, patch)
2011-02-14 18:22 PST, sam morris
mkanat: review+
Details | Diff | Review

Description sam morris 2011-02-14 17:24:25 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.13 (KHTML, like Gecko) Chrome/9.0.597.84 Safari/534.13
Build Identifier: 4.0rc2

The query assembled by Bugzilla::Bug's possible_duplicates function fails on PostgreSQL with an error similar to the following:

ERROR:  argument of OR must be type boolean, not type integer

Looking at the query:

SELECT bugs.bug_id AS bug_id,
    bugs.resolution AS resolution,
    (
        CASE WHEN (LOWER(bugs_fulltext.short_desc) LIKE '%security%') THEN 1 ELSE 0 END
        + CASE WHEN (LOWER(bugs_fulltext.short_desc) LIKE '%samba%') THEN 1 ELSE 0 END
    ) AS relevance
FROM bugs
INNER JOIN bugs_fulltext
    ON bugs.bug_id = bugs_fulltext.bug_id
WHERE
    (
        CASE WHEN (LOWER(bugs_fulltext.short_desc) LIKE '%security%') THEN 1 ELSE 0 END
        OR CASE WHEN (LOWER(bugs_fulltext.short_desc) LIKE '%samba%') THEN 1 ELSE 0 END
    )
AND product_id IN (2);

The problem is that PostgreSQL is strict about the types of operands that it accepts for the OR in the WHERE.

This is noted in the 4.0 release notes, but I couldn't find a bug about it, so here we are.

Reproducible: Always
Comment 1 Max Kanat-Alexander 2011-02-14 17:25:18 PST
Actually, the problem in the Release Notes is not this--the problem in the Release Notes is that even if this SQL did work (which it does in all previous versions of PostgreSQL), it does not actually return any results.
Comment 2 Max Kanat-Alexander 2011-02-14 17:27:53 PST
Sam: If you want to *fully* fix this so that Pg really *does* work with possible_duplicates, the best ultimate resolution would be to fix bug 365258.
Comment 3 sam morris 2011-02-14 17:31:24 PST
Created attachment 512358 [details] [diff] [review]
workaround

I've fixed this in my installation by co-ercing the type of each term in the query's WHERE to boolean by comparing the number returned by sql_fulltext_search with 1.

At first glance this seems a bit ugly, but the advantages of doing it this way are that sql_fulltext_search doesn't have to be modified; and that the result should still be portable to other databases that don't specialize sql_fulltext_search.
Comment 4 Max Kanat-Alexander 2011-02-14 17:32:24 PST
Comment on attachment 512358 [details] [diff] [review]
workaround

That's actually something that needs to be fixed in sql_fulltext_search, if it's not using a real boolean comparison.
Comment 5 sam morris 2011-02-14 17:57:23 PST
(In reply to comment #4)
> That's actually something that needs to be fixed in sql_fulltext_search, if
> it's not using a real boolean comparison.

Unfortunately this breaks the nice thing about sql_fulltext_search, which is that, for each search term, it returns one SQL expression that may be used both for matching and relevance sorting.

The problem isn't in sql_fulltext_search itself--but in how possible_duplicates stitches together the terms to form its own query. It assumes that you can freely cast from integer to boolean (which, to be fair, you could until PostgreSQL 8.3).

It appears that Bugzilla/Search.pm does something similar to my patch--it appends "> 0" to the term returned by sql_fulltext_search before using it in a WHERE clause.
Comment 6 Max Kanat-Alexander 2011-02-14 18:02:38 PST
(In reply to comment #5)
> It appears that Bugzilla/Search.pm does something similar to my patch--it
> appends "> 0" to the term returned by sql_fulltext_search before using it in a
> WHERE clause.

  Oh, it does? Okay, in that case, give me a patch that does > 0.
Comment 7 sam morris 2011-02-14 18:22:29 PST
Created attachment 512373 [details] [diff] [review]
modifying sql_fulltext_search instead

Modify sql_fulltext_search to return two values. The first is a boolean expression to be used in WHERE clases, the second is a numberic expression to be used in relevance ranking.

While this works with possible_duplicates, it breaks _content_matches in Bugzilla/Search.pm, which makes the opposite assumption: that sql_fulltext_search returns a numeric expression! I've fixed that and updated the documentation. It appears that this won't break MySQL and Oracle's own version of sql_fulltext_search, but I don't use either of those DBs so I'm not certain.
Comment 8 Max Kanat-Alexander 2011-02-14 18:23:52 PST
Comment on attachment 512373 [details] [diff] [review]
modifying sql_fulltext_search instead

Oh, this is clever, I like this. It will take a bit of cross-database testing, but I really wanted to remove that > comparator for performance reasons, so this looks really promising.
Comment 9 Max Kanat-Alexander 2011-02-28 18:24:14 PST
Comment on attachment 512373 [details] [diff] [review]
modifying sql_fulltext_search instead

Looks pretty awesome to me. I'm going to fix the POD very slightly on checkin.
Comment 10 Max Kanat-Alexander 2011-03-01 05:44:46 PST
Hey, the patch does actually make possible_duplicates work!! :-) That's awesome.

This required a little bitrot fix to apply on trunk, so I did that before checkin. Thank you so much for the patch, Sam, it was really excellent! :-)

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/DB.pm
modified Bugzilla/Search.pm
Committed revision 7726

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/                         
modified Bugzilla/DB.pm
modified Bugzilla/Search.pm
modified template/en/default/pages/release-notes.html.tmpl
Committed revision 7562.

Note You need to log in before you can comment on or make changes to this bug.