286686 - ORDER BY column needs to be selected or grouped

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Tomas Kopal]

For normal selects, Postgres requires columns, which are used in the ORDER BY
clause to be also in GROUP BY. For SELECT DISTINCT, columns used in ORDER BY
needs to be SELECTed.
Patch will follow

Comment 1

[Tomas Kopal]

Attachment: V1

In the course of fixing this, I was actually able to get rid of one query in
editusers.cgi, yupeee :-).

Comment 2

[Marc Schumann [:Wurblzap]]

>-    my $query = 'SELECT DISTINCT userid, login_name, realname, disabledtext ' .
>-                'FROM profiles';
>+    my $query = 'SELECT DISTINCT userid, login_name, realname, ' .
>+                'disabledtext, login_name FROM profiles';

I'm no Pg guy, but this seems strange to me... Do you really need to read
login_name twice?

Comment 3

[Tomas Kopal]

(In reply to comment #2)
> >-    my $query = 'SELECT DISTINCT userid, login_name, realname, disabledtext ' .
> >-                'FROM profiles';
> >+    my $query = 'SELECT DISTINCT userid, login_name, realname, ' .
> >+                'disabledtext, login_name FROM profiles';
> 
> I'm no Pg guy, but this seems strange to me... Do you really need to read
> login_name twice?

Ummmm, no? :-) Sorry, my oversight, I will fix that, thanks.

Comment 4

[Tomas Kopal]

Attachment: V1.1

Fixed comment #2.

Comment 5

[Tomas Kopal]

Attachment: V1.2

Sorry for the spam, I was a bit trigger-happy. I removed the functional change,
but didn't remove change in formatting. Fixed.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 179645 [details] [diff] [review]
V1.2

Hrm... this patch seems to be missing the actual SQL change for editusers.cgi,
and so now any actual changes to that file as a part of this bug aren't
necessary, right?

Comment 7

[Tomas Kopal]

(In reply to comment #6)
> (From update of attachment 179645 [details] [diff] [review] [edit])
> Hrm... this patch seems to be missing the actual SQL change for editusers.cgi,
> and so now any actual changes to that file as a part of this bug aren't
> necessary, right?
> 

Well, yes, I suppose. But I am sure the change in that file is still a valid
improvement. Should I move it to a different bug then?

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> Well, yes, I suppose. But I am sure the change in that file is still a valid
> improvement. Should I move it to a different bug then?

  Sure. :-) I always like code cleanup. 

  And then you can ask wurblzap for review. :-)

Comment 9

[Tomas Kopal]

Attachment: V1.3

Removed editusers.cgi part of the patch to bug 289062.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 179660 [details] [diff] [review]
V1.3

Great! This is now obviously correct. :-)

Comment 11

[Max Kanat-Alexander]

Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <-- 
summarize_time.cginew revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.51; previous revision: 1.50
done