Moodle

PostgreSQL error in survey + recent activity block

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.9
  • Fix Version/s: 1.9.1
  • Component/s: Survey
  • Labels:
    None
  • Environment:
    MOODLE_19_STABLE / PostgreSQL 8.1
  • Database:
    PostgreSQL
  • Affected Branches:
    MOODLE_19_STABLE
  • Fixed Branches:
    MOODLE_19_STABLE

Description

This appears in my Recent activity block:

ERROR: column "sa.time" must appear in the GROUP BY clause or be used in an aggregate function

SELECT sa.userid, sa.survey, sa.time, u.firstname, u.lastname, u.email, u.picture FROM mdl_survey_answers sa JOIN mdl_user u ON u.id = sa.userid WHERE sa.survey IN (9) AND sa.time > 1209238600 GROUP BY sa.userid, sa.survey ORDER BY sa.id ASC

  • line 686 of lib/dmllib.php: call to debugging()
  • line 162 of mod/survey/lib.php: call to get_recordset_sql()
  • line 942 of course/lib.php: call to survey_print_recent_activity()
  • line 27 of blocks/recent_activity/block_recent_activity.php: call to print_recent_activity()
  • line 317 of blocks/moodleblock.class.php: call to block_recent_activity->get_content()
  • line 341 of blocks/moodleblock.class.php: call to block_base->is_empty()
  • line 338 of lib/blocklib.php: call to block_base->_print_block()
  • line 284 of course/format/topics/format.php: call to blocks_print_group()
  • line 226 of course/view.php: call to require()

Issue Links

This issue is duplicated by:
MDL-14669 sql error in survey_print_recent_activity Major Closed
This issue has been marked as being related by:
MDL-12945 recent activity and separate mode cleanup and fixes - meta Blocker Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
David Mudrak added a comment -

The patch works at PostgreSQL - please test on other DBs

Show
David Mudrak added a comment - The patch works at PostgreSQL - please test on other DBs
Hide
Permalink
David Mudrak added a comment -

Eloy, please, can you look at this? Thanks

Show
David Mudrak added a comment - Eloy, please, can you look at this? Thanks
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Hi David,

IMO, if we haven't any aggregation in the SELECT clause (i.e. count max()....), then must not group by anything. I would use one simpler DISTINCT clause instead of the whole GROUP BY.

Ciao (and thanks!)

Show
Eloy Lafuente (stronk7) added a comment - Hi David, IMO, if we haven't any aggregation in the SELECT clause (i.e. count max()....), then must not group by anything. I would use one simpler DISTINCT clause instead of the whole GROUP BY. Ciao (and thanks!)
Hide
Permalink
David Mudrak added a comment -

Hi Eloy,

I don't think so. IMHO the intention of the query author was to get results in a form of

userid | survey | time | firstname | lastname | email | picture

where for given userid and survey, only one (most recent) row is displayed. But this relies on MySQL-specific behaviour only and does not work with PostgreSQL.
If I used SELECT DISTINCT, I would get all rows because they can be distinguished by the "time" attribute. So, DISTINCT does not matter here.

Show
David Mudrak added a comment - Hi Eloy, I don't think so. IMHO the intention of the query author was to get results in a form of userid | survey | time | firstname | lastname | email | picture where for given userid and survey, only one (most recent) row is displayed. But this relies on MySQL-specific behaviour only and does not work with PostgreSQL. If I used SELECT DISTINCT, I would get all rows because they can be distinguished by the "time" attribute. So, DISTINCT does not matter here.
Hide
Permalink
David Mudrak added a comment -

To clarify: the patch behaves completely in the same way as the DISTINCT described above and therefore it is not good. My question is how to achieve the desired functionality "get the one most recent record per user per survey"?

Show
David Mudrak added a comment - To clarify: the patch behaves completely in the same way as the DISTINCT described above and therefore it is not good. My question is how to achieve the desired functionality "get the one most recent record per user per survey"?
Hide
Permalink
Petr Škoda (skodak) added a comment -

MAX(sa.time) ?

Show
Petr Škoda (skodak) added a comment - MAX(sa.time) ?
Hide
Permalink
David Mudrak added a comment -

Yes, I can use MAX(). But then I am back in GROUP BY, because I need to GROUP all records BY all the other column being selected in the query (column must appear in the GROUP BY clause or be used in an aggregate function).

Show
David Mudrak added a comment - Yes, I can use MAX(). But then I am back in GROUP BY, because I need to GROUP all records BY all the other column being selected in the query (column must appear in the GROUP BY clause or be used in an aggregate function).
Hide
Permalink
David Mudrak added a comment -

For me, this seems to be the way:

SELECT sa.userid, sa.survey, MAX(sa.time) AS time, u.firstname, u.lastname, u.email, u.picture
FROM {$CFG->prefix}survey_answers sa
JOIN {$CFG->prefix}user u ON u.id = sa.userid
WHERE sa.survey IN ($slist) AND sa.time > $timestart
GROUP BY sa.userid, sa.survey, u.firstname, u.lastname, u.email, u.picture
ORDER BY time ASC

Show
David Mudrak added a comment - For me, this seems to be the way: SELECT sa.userid, sa.survey, MAX(sa.time) AS time, u.firstname, u.lastname, u.email, u.picture FROM {$CFG->prefix}survey_answers sa JOIN {$CFG->prefix}user u ON u.id = sa.userid WHERE sa.survey IN ($slist) AND sa.time > $timestart GROUP BY sa.userid, sa.survey, u.firstname, u.lastname, u.email, u.picture ORDER BY time ASC
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

ah, yes (I didn't know that sa.time could be different for one user in the same survey).

So MAX(time) is correct, and if you use aggregations then you need to group by everything else. yup (that's the only one cross-db solution).

Thanks! Ciao

Show
Eloy Lafuente (stronk7) added a comment - ah, yes (I didn't know that sa.time could be different for one user in the same survey). So MAX(time) is correct, and if you use aggregations then you need to group by everything else. yup (that's the only one cross-db solution). Thanks! Ciao
Hide
Permalink
David Mudrak added a comment -

Thank you for clarification, Eloy. On the other hand - you are probably right. I did not realize the Survey module stores just one answer per user. So in this case, DISTINCT would be just fine.
Just for case, I propose to put MAX() based fix to MOODLE_19_STABLE and DISTINCT based improvement to HEAD. If I receive +1 from you for this proposal, I will prepare patches.

Show
David Mudrak added a comment - Thank you for clarification, Eloy. On the other hand - you are probably right. I did not realize the Survey module stores just one answer per user. So in this case, DISTINCT would be just fine. Just for case, I propose to put MAX() based fix to MOODLE_19_STABLE and DISTINCT based improvement to HEAD. If I receive +1 from you for this proposal, I will prepare patches.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

+1 to use your original proposed MAX(sa.time) + group by everything else (both under 19_STABLE and HEAD - and backporting to 18_STABLE IMO).

Although sa.time will be the same (save.php, line 67) I think we shouldn't rely on that "feature" (imagine someone implements editing of answers). So +1 to be in the safe side and use MAX().

Ciao

Show
Eloy Lafuente (stronk7) added a comment - +1 to use your original proposed MAX(sa.time) + group by everything else (both under 19_STABLE and HEAD - and backporting to 18_STABLE IMO). Although sa.time will be the same (save.php, line 67) I think we shouldn't rely on that "feature" (imagine someone implements editing of answers). So +1 to be in the safe side and use MAX(). Ciao
Hide
Permalink
David Mudrak added a comment -

The attached MDL-14583.patch2.txt fixes the problem for HEAD and MOODLE_19_STABLE

There is no need to backport because in MOODLE_18_STABLE, the recent activity is read by get_records_select() from 'mdl_log' table. This was rewritten by skodak as a part of MDL-12945 @ rev. 1.66 and 1.63.2.3

I do not have cvs write access, please commit and resolve. Thanks

Show
David Mudrak added a comment - The attached MDL-14583.patch2.txt fixes the problem for HEAD and MOODLE_19_STABLE There is no need to backport because in MOODLE_18_STABLE, the recent activity is read by get_records_select() from 'mdl_log' table. This was rewritten by skodak as a part of MDL-12945 @ rev. 1.66 and 1.63.2.3 I do not have cvs write access, please commit and resolve. Thanks
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Thanks a lot David!

patch applied to 19_STABLE and HEAD.

Resolving as fixed. Ciao

Show
Eloy Lafuente (stronk7) added a comment - Thanks a lot David! patch applied to 19_STABLE and HEAD. Resolving as fixed. Ciao
Hide
Permalink
Petr Škoda (skodak) added a comment -

thanks, closing

Show
Petr Škoda (skodak) added a comment - thanks, closing

People

Vote (1)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.5#665-sha1:422aead) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.