Calling possible_duplicates throws an error about invalid SQL on PostgreSQL 8.4

RESOLVED FIXED in Bugzilla 4.0

Status

()

Bugzilla
Database
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: sam morris, Assigned: sam morris)

Tracking

Bugzilla 4.0
Bug Flags:
approval +
approval4.0 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.70 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
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

7 years ago
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

7 years ago
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.

Updated

7 years ago
Version: unspecified → 4.0
(Assignee)

Comment 3

7 years ago
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.
Attachment #512358 - Flags: review?(mkanat)

Comment 4

7 years ago
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-

Updated

7 years ago
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

Updated

7 years ago
Assignee: database → sam
(Assignee)

Comment 5

7 years ago
(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

7 years ago
(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.
(Assignee)

Comment 7

7 years ago
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.
Attachment #512373 - Flags: review?(mkanat)

Comment 8

7 years ago
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

7 years ago
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+

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

7 years ago
Attachment #512358 - Attachment is obsolete: true

Updated

7 years ago
Flags: approval4.0+
Flags: approval+

Comment 10

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.