Non-core contributed modules

non postgres friendly sql in messages block

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Deferred
  • Affects Version/s: 1.8.2
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Environment:
    BSDI

Description

The following sql is not valid in postgres (in fact, it's not valid at all, but mysql allows it incorrectly)

in blocks/messages/block_messages.php in the get_content function

SELECT m.useridfrom as id, COUNT(m.useridfrom) as count, u.firstname, u.lastname, u.picture, u.lastaccess FROM mdl_user u, mdl_message m WHERE m.useridto = '1' AND u.id = m.useridfrom GROUP BY m.useridfrom

You can't put fields in the select list that aren't in the GROUP by field.

I'm not familiar with the IM code but I'll try and fix this if I get time. If someone wants to tell me what the desired behaviour is, that would be helpful.

Activity

Hide
Martin Dougiamas added a comment -

From (penny at catalyst.net.nz) Thursday, 24 February 2005, 10:27 AM:

Sorry, to be more helpful, here is the error message:

ERROR: column u.firstname must appear in the GROUP BY clause or be used in an aggregate function

From (penny at catalyst.net.nz) Thursday, 24 February 2005, 10:29 AM:

And some useful links explaining the problem with this query

http://archives.postgresql.org/pgsql-general/2004-02/msg01199.php

http://www.phpbuilder.com/board/showthread.php?threadid=10232136

From (penny at catalyst.net.nz) Thursday, 24 February 2005, 10:34 AM:

I've attached a patch that fixes the sql problem, but not sure whether the functionality is intact.

From Martin Dougiamas (martin at moodle.com) Thursday, 24 February 2005, 10:47 AM:

Thanks, Penny!!

This code is just meant to return a list of all people who have sent you recent messages, along with a count of their messages.

> You

can't put fields in the select list that aren't in the GROUP by field.

Surely it's OK when we know all the grouped data is the same anyway (like in this case) .... gah, stupid databases.

The patch will work but all that extra get_record work bothers me ... can we just add the extra fields to GROUP BY?

I've check this into CVS so you can try it out on PG.

From (penny at catalyst.net.nz) Thursday, 24 February 2005, 11:02 AM:

Even despite the cheeky commit message () I tested it and yup, it works fine

The reason you normally wouldn't do this is because there is no guarantee that the rest of the fields are going to be the same, so how does the database know which ones to return? I have no idea what mysql does in that case, but I don't think it's reliable.

In this case, you're right, it is going to be the same, and your solution is better for not using extra queries.

Closing bug!

Show
Martin Dougiamas added a comment - From (penny at catalyst.net.nz) Thursday, 24 February 2005, 10:27 AM: Sorry, to be more helpful, here is the error message: ERROR: column u.firstname must appear in the GROUP BY clause or be used in an aggregate function From (penny at catalyst.net.nz) Thursday, 24 February 2005, 10:29 AM: And some useful links explaining the problem with this query http://archives.postgresql.org/pgsql-general/2004-02/msg01199.php http://www.phpbuilder.com/board/showthread.php?threadid=10232136 From (penny at catalyst.net.nz) Thursday, 24 February 2005, 10:34 AM: I've attached a patch that fixes the sql problem, but not sure whether the functionality is intact. From Martin Dougiamas (martin at moodle.com) Thursday, 24 February 2005, 10:47 AM: Thanks, Penny!! This code is just meant to return a list of all people who have sent you recent messages, along with a count of their messages. > You can't put fields in the select list that aren't in the GROUP by field. Surely it's OK when we know all the grouped data is the same anyway (like in this case) .... gah, stupid databases. The patch will work but all that extra get_record work bothers me ... can we just add the extra fields to GROUP BY? I've check this into CVS so you can try it out on PG. From (penny at catalyst.net.nz) Thursday, 24 February 2005, 11:02 AM: Even despite the cheeky commit message () I tested it and yup, it works fine The reason you normally wouldn't do this is because there is no guarantee that the rest of the fields are going to be the same, so how does the database know which ones to return? I have no idea what mysql does in that case, but I don't think it's reliable. In this case, you're right, it is going to be the same, and your solution is better for not using extra queries. Closing bug!
Hide
Michael Blake added a comment -

Reassinging to a valid user. Cleanup project.

Show
Michael Blake added a comment - Reassinging to a valid user. Cleanup project.
Hide
Eloy Lafuente (stronk7) added a comment -

properly marked as "resolved" this (general cleanup)

Show
Eloy Lafuente (stronk7) added a comment - properly marked as "resolved" this (general cleanup)

People

Dates

  • Created:
    Updated:
    Resolved: