Closed Bug 634144 Opened 13 years ago Closed 13 years ago

Calling possible_duplicates throws an error about invalid SQL on PostgreSQL 8.4

Categories

(Bugzilla :: Database, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: sam, Assigned: sam)

Details

Attachments

(1 file, 1 obsolete file)

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.
Version: unspecified → 4.0
Attached patch workaround (obsolete) — Splinter Review
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
Assignee: database → sam
(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+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #512358 - Attachment is obsolete: true
Flags: approval4.0+
Flags: approval+
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: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.