FlagType::match uses a HAVING clause that PostgreSQL does not support

RESOLVED FIXED in Bugzilla 2.20

Status

()

P1
normal
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: mkanat, Assigned: Tomas.Kopal)

Tracking

2.19.2
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

14 years ago
Apparently, PostgreSQL does not support having an alias in the HAVING clause.

Is there some way that we can restructure this code? I'm a bit lost here.

Here's a cleaned-up version of the error:

DBD::Pg::st execute failed: ERROR: Attribute "num_exclusions" not found

SELECT 1, flagtypes.id, flagtypes.name, flagtypes.description, 
flagtypes.cc_list, flagtypes.target_type, flagtypes.sortkey, 
flagtypes.is_active, flagtypes.is_requestable, flagtypes.is_requesteeble,
 flagtypes.is_multiplicable, flagtypes.grant_group_id, 
flagtypes.request_group_id, COUNT(flagexclusions.type_id) AS num_exclusions 
FROM flagtypes 
  INNER JOIN flaginclusions ON flagtypes.id = flaginclusions.type_id 
  LEFT JOIN flagexclusions ON (flagtypes.id = flagexclusions.type_id
            AND (flagexclusions.product_id = 1 
                 OR flagexclusions.product_id IS NULL)
            AND (flagexclusions.component_id = 1 
                 OR flagexclusions.component_id IS NULL)) 
WHERE 1=1 
      AND flagtypes.target_type = 'b' 
      AND (flaginclusions.product_id = 1 OR flaginclusions.product_id IS NULL)
      AND (flaginclusions.component_id = 1 
           OR flaginclusions.component_id IS NULL) 
GROUP BY flagtypes.id, flagtypes.name, flagtypes.description, 
         flagtypes.cc_list, flagtypes.target_type, flagtypes.sortkey, 
         flagtypes.is_active, flagtypes.is_requestable, 
         flagtypes.is_requesteeble, flagtypes.is_multiplicable, 
         flagtypes.grant_group_id, flagtypes.request_group_id
HAVING num_exclusions = 0 
ORDER BY flagtypes.sortkey, flagtypes.name

at Bugzilla/DB.pm line 69 
Bugzilla::DB::SendSQL('SELECT 1, flagtypes.id, flagtypes.name,
flagtypes.description...') called at Bugzilla/FlagType.pm line 148 
Bugzilla::FlagType::match('HASH(0x8f000d0)') called at Bugzilla/Bug.pm line 325 
Bugzilla::Bug::flag_types('Bugzilla::Bug=HASH(0x8e1e678)') called at
data/template/template/en/default/bug/edit.html.tmpl line 390
...
Template::process('Bugzilla::Template=HASH(0x8c81e68)','bug/create/created.html.tmpl','HASH(0x8c4ca5c)')
called at /var/www/html/dbinstall/post_bug.cgi line 523
(Reporter)

Updated

14 years ago
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.20
HAVING is a restriction that happens in the result set after the query has run
and the result set has been generated.  A PostGreSQL-specific version of this
query could do the same thing in Perl after retrieving the result set but before
returning it to the code which requested it.
(Assignee)

Comment 2

14 years ago
The simplest is to just change "HAVING num_exclusions = 0" to "HAVING
COUNT(flagexclusions.type_id) = 0", if possible...
(Reporter)

Comment 3

14 years ago
I talked with myk in IRC, also, and looked at the Pg docs, and this is what I
came up with. :-) It works on Pg.
Assignee: attach-and-request → mkanat
Status: NEW → ASSIGNED
Attachment #177081 - Flags: review?(Tomas.Kopal)

Comment 4

14 years ago
The having goes away altogether if you fix the SQL so that it does the join
properly.

The flagtypes joins should be left joins and put a "xxx IS NOT NULL" in the
wherepart.  That will make boolean charts with flags work properly (they
currently do not, especially with negation) and will eliminate the HAVING issue
altogether.

(Assignee)

Comment 5

14 years ago
Comment on attachment 177081 [details] [diff] [review]
Fix the HAVING clause to be Pg-compatible

As per Joels comment.
Attachment #177081 - Flags: review?(Tomas.Kopal) → review-
(Reporter)

Comment 6

14 years ago
Actually, I'd like to just do this Pg fix for now, and do the better fix later.
Flags are cryptic, so they tend to take up a lot of my time just trying to
figure them out and test them.

I believe that we already have a bug filed for the better fix, yes?
(Assignee)

Comment 7

14 years ago
Posted patch V2 (obsolete) — Splinter Review
What about this, is this what you were expecting, Joel? It actually cleaned the
code a bit and removed few GROUP BYs :-). Tested on Postgres only.
Attachment #177081 - Attachment is obsolete: true
Attachment #177108 - Flags: review?(bugreport)
(Reporter)

Updated

14 years ago
Assignee: mkanat → Tomas.Kopal
Status: ASSIGNED → NEW

Comment 8

14 years ago
Comment on attachment 177108 [details] [diff] [review]
V2


DBD::mysql::st execute failed: Mixing of GROUP columns (MIN(),MAX(),COUNT()...)
with no GROUP columns is illegal if there is no GROUP BY clause [for Statement
"SELECT 1, flagtypes.id, flagtypes.name, flagtypes.description,
flagtypes.cc_list, flagtypes.target_type, flagtypes.sortkey,
flagtypes.is_active, flagtypes.is_requestable, flagtypes.is_requesteeble,
flagtypes.is_multiplicable, flagtypes.grant_group_id,
flagtypes.request_group_id, COUNT(flags.id) FROM flagtypes LEFT OUTER JOIN
flags ON flagtypes.id = flags.type_id WHERE 1=1 AND flagtypes.target_type = 'b'
ORDER BY flagtypes.sortkey, flagtypes.name"] at Bugzilla/DB.pm line 69
	Bugzilla::DB::SendSQL('SELECT 1, flagtypes.id, flagtypes.name,
flagtypes.description...') called at Bugzilla/FlagType.pm line 142
	Bugzilla::FlagType::match('HASH(0x86da064)',1) called at
xxx/editflagtypes.cgi line 94
	main::list() called at xxxx/editflagtypes.cgi line 73
Attachment #177108 - Flags: review?(bugreport) → review-
(Assignee)

Comment 9

14 years ago
Posted patch V2.1 (obsolete) — Splinter Review
Ok, I was a bit trigger-happy when cleaning up, sorry :-). Fixed.
Attachment #177108 - Attachment is obsolete: true
Attachment #177351 - Flags: review?(bugreport)

Comment 10

14 years ago
Comment on attachment 177351 [details] [diff] [review]
V2.1

r=joel if you've tested it
Attachment #177351 - Flags: review?(bugreport) → review+
(Assignee)

Comment 11

14 years ago
(In reply to comment #10)
> (From update of attachment 177351 [details] [diff] [review] [edit])
> r=joel if you've tested it
> 

Yes, I tested it on both Postgres and MySQL.
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Flags: approval?
Comment on attachment 177351 [details] [diff] [review]
V2.1

You need to update the comment as well to no longer reference use of a HAVING
clause.  Also, there's something that makes me uneasy about this fix.  As I
recall, there was a reason I used HAVING here, although I can't think of it
now.  We should be very careful about watching Bugzilla after making this
change to make sure we haven't introduced any regressions.
Attachment #177351 - Flags: review-
(Reporter)

Comment 13

14 years ago
CC'ing our regression-finding experts. :-)
(Assignee)

Updated

14 years ago
Flags: approval?
(Assignee)

Comment 14

14 years ago
Posted patch V2.2 (obsolete) — Splinter Review
(In reply to comment #12)
> (From update of attachment 177351 [details] [diff] [review] [edit])
> You need to update the comment as well to no longer reference use of a HAVING

> clause.

Ahhh, good catch, thanks. Fixed.

>  Also, there's something that makes me uneasy about this fix.  As I
> recall, there was a reason I used HAVING here, although I can't think of it
> now.	We should be very careful about watching Bugzilla after making this
> change to make sure we haven't introduced any regressions.

Pitty you don't remember it now. As far as I can tell, this should be
functionally identical (and I spent a lot of time thinking how it did work and
how it works now :-) ). So as you say, let's keep an eye on flags after this
lands and lets hope your feeling will not come true :-).
Attachment #177351 - Attachment is obsolete: true
Attachment #177576 - Flags: review?(myk)
Attachment #177576 - Flags: review?(myk) → review+
Flags: approval+
(Assignee)

Comment 15

14 years ago
Just unbitrotted, no other change.
Attachment #177576 - Attachment is obsolete: true
(Reporter)

Comment 16

14 years ago
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.13; previous revision: 1.12
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Blocks: 291539
You need to log in before you can comment on or make changes to this bug.