Closed Bug 634144 Opened 10 years ago Closed 10 years ago
_duplicates throws an error about invalid SQL on Postgre SQL 8 .4
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
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.
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.
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.
Attachment #512358 - Flags: review?(mkanat)
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.
Attachment #512358 - Flags: review?(mkanat) → review-
Summary: fix possible_duplicates to work with PostgreSQL → Calling possible_duplicates throws an error about invalid SQL on PostgreSQL 8.4
Target Milestone: --- → Bugzilla 4.0
(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.
(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.
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.
Attachment #512373 - Flags: review?(mkanat)
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 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.
Attachment #512373 - Flags: review?(mkanat) → review+
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.