330365 - [PostgreSQL] Crash when changing the displayed columns after a "specific" search

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Max Kanat-Alexander]

A landfill user reported that she got the SQL error 'ORDER BY "relevance" is ambiguous' using today's cvs tip. Examining the SQL, I see in the SELECT statement:

(SUM(CASE WHEN
       (LOWER(longdescs_0.thetext) LIKE '%triad%') THEN 1 ELSE 0
       END)/COUNT(CASE WHEN (LOWER(longdescs_0.thetext) LIKE '%triad%') THEN 1
       ELSE 0 END) + CASE WHEN (LOWER(bugs.short_desc) LIKE '%triad%') THEN 1
       ELSE 0 END) AS relevance, 


(SUM(CASE WHEN (LOWER(longdescs_1.thetext)
       LIKE '%triad%') THEN 1 ELSE 0 END)/COUNT(CASE WHEN
       (LOWER(longdescs_1.thetext) LIKE '%triad%') THEN 1 ELSE 0 END) + CASE
       WHEN (LOWER(bugs.short_desc) LIKE '%triad%') THEN 1 ELSE 0 END) A
       S relevance, (SUM(CASE WHEN (LOWER(longdescs_2.thetext) LIKE '%triad%')
       THEN 1 ELSE 0 END)/COUNT(CASE WHEN (LOWER(longdescs_2.thetext) LIKE
       '%triad%') THEN 1 ELSE 0 END) + CASE WHEN (LOWER(bugs.short_desc) LIKE
       '%triad%') THEN 1 ELSE 0 END) AS relevance 

She said she was coming from colchange.cgi. So somehow it's possible to generate two "relevance" fields, which is bad, but not easy to reproduce.

Comment 1

[Max Kanat-Alexander]

Okay, looking at the WHERE clause some more, I see a few parts like:

CASE WHEN (LOWER(longdescs_0.thetext) LIKE '%triad%') THEN 1 ELSE 0 END > 0
  OR bugs.short_desc ~* '(^|[^a-z0-9])triad($|[^a-z0-9])')

and:

CASE WHEN (LOWER(longdescs_1.thetext) LIKE '%triad%') THEN 1 ELSE 0 END > 0 
  OR bugs.short_desc ~* '(^|[^a-z0-9])triad($|[^a-z0-9])'

and:

CASE WHEN (LOWER(longdescs_2.thetext) LIKE '%triad%') THEN 1 ELSE 0 END > 0
  OR bugs.short_desc ~* '(^|[^a-z0-9])triad($|[^a-z0-9])'

Notice anything strange about that? It's the same boolean chart three times. Do we actually allow that?

Comment 2

[Frédéric Buclin]

It's trivial to reproduce on PostgreSQL, where we get a crash:

1. Go to query.cgi?format=specific (Find a Specific Bug)
2. Enter a word, such as "bugzilla"
3. Click "Search"
4. Under the bug list, click the "Change Columns" link
5. Change your list of columns if you want, but that's optional (you will crash in both cases)
6. Click the "Change Columns" button
7. Instead of displaying your buglist again, you get:

DBD::Pg::st execute failed: ERROR:  ORDER BY "relevance" is ambiguous
 [for Statement "SELECT bugs.bug_id, bugs.bug_severity, bugs.priority, bugs.bug_status, bugs.resolution, map_products.name, bugs.delta_ts, map_assigned_to.login_name, map_reporter.login_name, bugs.bug_status, bugs.resolution, bugs.short_desc, (SUM(CASE WHEN (LOWER(longdescs_0.thetext) LIKE '%political%') THEN 1 ELSE 0 END)/COUNT(CASE WHEN (LOWER(longdescs_0.thetext) LIKE '%political%') THEN 1 ELSE 0 END) + CASE WHEN (LOWER(bugs.short_desc) LIKE '%political%') THEN 1 ELSE 0 END) AS relevance, (SUM(CASE WHEN (LOWER(longdescs_1.thetext) LIKE '%political%') THEN 1 ELSE 0 END)/COUNT(CASE WHEN (LOWER(longdescs_1.thetext) LIKE '%political%') THEN 1 ELSE 0 END) + CASE WHEN (LOWER(bugs.short_desc) LIKE '%political%') THEN 1 ELSE 0 END) AS relevance FROM bugs  INNER JOIN profiles AS map_assigned_to ON (bugs.assigned_to = map_assigned_to.userid) INNER JOIN profiles AS map_reporter ON (bugs.reporter = map_reporter.userid) INNER JOIN products AS map_products ON (bugs.product_id = map_products.id) INNER JOIN longdescs AS longdescs_0 ON (bugs.bug_id = longdescs_0.bug_id ) INNER JOIN longdescs AS longdescs_1 ON (bugs.bug_id = longdescs_1.bug_id ) LEFT JOIN bug_group_map  ON bug_group_map.bug_id = bugs.bug_id  AND bug_group_map.group_id NOT IN (8,4,16,20,9,14,18,2,17,12,3,10,11,13,21,1,6,15,5,7)  LEFT JOIN cc ON cc.bug_id = bugs.bug_id AND cc.who = 10195 WHERE ((bugs.bug_status IN ('UNCONFIRMED','NEW','ASSIGNED','REOPENED'))) AND (((CASE WHEN (LOWER(longdescs_0.thetext) LIKE '%political%') THEN 1 ELSE 0 END > 0) OR (bugs.short_desc ~* '(^|[^a-z0-9])political($|[^a-z0-9])'))) AND (((CASE WHEN (LOWER(longdescs_1.thetext) LIKE '%political%') THEN 1 ELSE 0 END > 0) OR (bugs.short_desc ~* '(^|[^a-z0-9])political($|[^a-z0-9])'))) AND bugs.creation_ts IS NOT NULL AND ((bug_group_map.group_id IS NULL)    OR (bugs.reporter_accessible = 1 AND bugs.reporter = 10195)     OR (bugs.cclist_accessible = 1 AND cc.who IS NOT NULL)     OR (bugs.assigned_to = 10195) OR (bugs.qa_contact = 10195) ) GROUP BY bugs.bug_id, bugs.short_desc, bugs.short_desc, bugs.bug_severity, bugs.priority, bugs.bug_status, bugs.resolution, map_products.name, bugs.delta_ts, map_assigned_to.login_name, map_reporter.login_name ORDER BY relevance desc LIMIT 200"] at /var/www/html/qa222pg/buglist.cgi line 917


I cannot reproduce on tip, where 'AS relevance' appears only once, nor can I reproduce on 2.22 using MySQL, despite 'AS relevance' appears twice too.

I consider this as a blocker as 1) we officially support PosgreSQL since 2.22, and 2) the "Find a specific bug" search page is the one by default for users with no privs.

Comment 3

[Frédéric Buclin]

joel, could you have a look at this bug? I would really like to see it fixed for 2.22.1 as this is our latest stable release.

Comment 4

[Max Kanat-Alexander]

This may be a regression since 2.20, but it's not a regression since 2.22, so I don't think it's appropriate to block the release on it at this point.

I'd certainly be fine with taking a patch for it before we release, though.

Comment 5

[Frédéric Buclin]

We didn't support Pg in 2.20.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> We didn't support Pg in 2.20.

  Yeah, true. I'm just saying that we released 2.22 with this bug in it, and nobody died. We can release 2.22.1 with this bug in it, for now. If you requested blocking 2.22.2 (if we had that flag), I'd grant it. And of course, if somebody fixes this before 2.22.1, then that's fine, too! :-)

Comment 7

[Max Kanat-Alexander]

All right. Now that we have this flag, it's granted, as I said I would do. :-)

Comment 8

[Dave Miller [:justdave]]

Attachment: Patch v1

OK, the issue here is that the Change Columns button is getting a copy of the query string after buglist.cgi and Search.pm have been messing with it, and Search.pm on the 2.22 branch adds the chart items for content by setting the $cgi->param() for them.  So once the Change Columns button gets it, the content stuff is in it twice, once as a content= param, and once as a 'content matches' boolean chart item.   On the tip, Search.pm just pushes them onto the in-memory chart array directly without futzing with $cgi->param().

The 2.22 branch has the in-memory chart array.  So the easy fix is to backport the tip's usage of the array push instead.  Looks like this got into the tip on bug 287170 as just a code cleanup in the process of doing other things.

Comment 9

[Dave Miller [:justdave]]

And for the record, I did test this, with it applied to qa222pg, and it worked. :) (but it's no longer applied there because I didn't want it to mess up any updates)

Comment 10

[Max Kanat-Alexander]

Comment on attachment 245074 [details] [diff] [review]
Patch v1

Yeah, sir, that looks fine to me. It does the same thing, and we know it works on the tip.

Comment 11

[Dave Miller [:justdave]]

Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.121.2.4; previous revision: 1.121.2.3
done