Error when viewing participant's posts page

Details

Type: Bug
Status: Closed
Priority: Critical
Resolution: Fixed
Affects Version/s: 2.2.4
Fix Version/s: 2.2.6, 2.3.3, STABLE backlog
Component/s: Forum
Labels:
- triaged

Database:

Microsoft SQL
Testing Instructions:
Hide

Repeat the following for all supported databases, especially Oracle and Mssql!

Log in as admin/teacher

Navigate to a course with students who have posted to the forums

Navigate to Navigation > CourseXXX > Participants

Select a participant

Navigate to Navigation > CourseXXX > Participants > ParticipantXXX > Forum posts > Posts

VERIFY: the results are accurate and no errrors are returned

Navigate to Navigation > CourseXXX > Participants > ParticipantXXX > Forum posts > Discussions

VERIFY: the results are accurate and no errrors are returned
Show
Repeat the following for all supported databases, especially Oracle and Mssql! Log in as admin/teacher Navigate to a course with students who have posted to the forums Navigate to Navigation > CourseXXX > Participants Select a participant Navigate to Navigation > CourseXXX > Participants > ParticipantXXX > Forum posts > Posts VERIFY: the results are accurate and no errrors are returned Navigate to Navigation > CourseXXX > Participants > ParticipantXXX > Forum posts > Discussions VERIFY: the results are accurate and no errrors are returned
Affected Branches:
MOODLE_22_STABLE
Fixed Branches:
MOODLE_22_STABLE, MOODLE_23_STABLE
Pull from Repository:
git://github.com/samhemelryk/moodle.git
Pull Master Branch:
wip-~~MDL-34527~~-m24
Pull Master Diff URL:
https://github.com/samhemelryk/moodle/compare/master...wip-MDL-34527-m24

Description

There is an error when you view the Posts and Discussions pages related to a course participant. This seems to happen in MS SQL and Oracle but not MySQL or PostgreSQL.

The problem is that we are using the DISTINCT qualifier on all fields from the forum table, which includes a long text field. We need to investigate what fields are actually needed from this query. I started tracing this back, but it will take some time to determine which fields are actually needed.

Default exception handler: Error reading from database Debug: The ntext data type cannot be selected as DISTINCT because it is not comparable.
SELECT DISTINCT f.*, cm.id AS cmid
            FROM mdl_forum f
            JOIN mdl_course_modules cm ON cm.instance = f.id
            JOIN mdl_modules m ON m.id = cm.module
            JOIN mdl_forum_discussions fd ON fd.forum = f.id
                    JOIN mdl_forum_posts fp ON fp.discussion = fd.id
            WHERE m.name = 'forum' AND f.course = ? AND fp.userid = ?
[array (
  0 => 2203,
  1 => '33713',
)]
* line 394 of \lib\dml\moodle_database.php: dml_read_exception thrown
* line 255 of \lib\dml\mssql_native_moodle_database.php: call to moodle_database->query_end()
* line 711 of \lib\dml\mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end()
* line 740 of \lib\dml\mssql_native_moodle_database.php: call to mssql_native_moodle_database->get_recordset_sql()
* line 7925 of \mod\forum\lib.php: call to mssql_native_moodle_database->get_records_sql()
* line 8078 of \mod\forum\lib.php: call to forum_get_forums_user_posted_in()
* line 121 of \mod\forum\user.php: call to forum_get_posts_by_user()

Replication steps:

Log in as admin/teacher
Navigate to a course with students who have posted to the forums
Navigate to Navigation > CourseXXX > Participants
Select a participant
Navigate to Navigation > CourseXXX > Participants > ParticipantXXX > Forum posts > Posts
Navigate to Navigation > CourseXXX > Participants > ParticipantXXX > Forum posts > Discussions

Expected result: The student's posts are reported

Actual result: With MS SQL and Oracle, an error is shown

The offending query is at around 8046 in /mod/forum/lib

Issue Links

has been marked as being related by

MDL-35513 Attempt to go to Forum > Posts under My Profile and error generated.

MDL-35936 Reports -> Security overview doese not work on Moodle 2.2+, MS-SQL 2008R2

is duplicated by

MDL-30118 Error when viewing users´ forum posts from the Navigation block (forum_get_courses_user_posted_in)

MDL-35505 The ntext data type cannot be selected as DISTINCT because it is not comparable

will help resolve

MDL-36759 MS SQL DB Read fail (ntext with a DISTINCT SQL Query)

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Fred Weiss added a comment - 25/Jul/12 7:17 AM

Changing field mdl_forum intro from ntext to nvarchar(max) seems to let the query run successfully.

I tried this on my test site.

Show

Fred Weiss added a comment - 25/Jul/12 7:17 AM Changing field mdl_forum intro from ntext to nvarchar(max) seems to let the query run successfully. I tried this on my test site.

Hide

Permalink

Michael de Raadt added a comment - 26/Jul/12 2:16 PM

Hi, Fred.

Could you please explain the circumstances that led to this error?

Also, what database are you using?

Converting a DB field is probably not going to be desirable, but we should be able to modify the query to work around this restriction.

Show

Michael de Raadt added a comment - 26/Jul/12 2:16 PM Hi, Fred. Could you please explain the circumstances that led to this error? Also, what database are you using? Converting a DB field is probably not going to be desirable, but we should be able to modify the query to work around this restriction.

Hide

Permalink

Michael de Raadt added a comment - 02/Aug/12 1:51 PM

Hi, Fred.

Could you please provide the additional information I asked for earlier?

Show

Michael de Raadt added a comment - 02/Aug/12 1:51 PM Hi, Fred. Could you please provide the additional information I asked for earlier?

Hide

Permalink

Michael de Raadt added a comment - 07/Aug/12 4:15 PM

Sam: You seem to be the last person to touch this. Perhaps you might like to look at it.

Show

Michael de Raadt added a comment - 07/Aug/12 4:15 PM Sam: You seem to be the last person to touch this. Perhaps you might like to look at it.

Hide

Permalink

Fred Weiss added a comment - 04/Sep/12 12:44 PM

Deleted my last post as it was incorrect. Following the replication steps above I got the error message listed above.
Replication steps:

Log in as admin/teacher
Navigate to a course with students who have posted to the forums
Navigate to Navigation > CourseXXX > Participants
Select a participant
Navigate to Navigation > CourseXXX > Participants > ParticipantXXX > Forum posts > Posts
Navigate to Navigation > CourseXXX > Participants > ParticipantXXX > Forum posts > Discussions

I checked in the tables mentioned and found two contained fields with the data type = ntext.

mdl_forum_posts - column name - message
mdl_forum - column name - intro

I changed both of these to type nvarchar(max) and the report works correctly.

This problem is reported a few other times in tracker and moodle.org forums in other areas. Each time it is recommended to make this change and it rectifies the problem.

The question was asked why use ntext data type at all. I am not however a DB analyst so can't really comment.

Show

Fred Weiss added a comment - 04/Sep/12 12:44 PM Deleted my last post as it was incorrect. Following the replication steps above I got the error message listed above. Replication steps: Log in as admin/teacher Navigate to a course with students who have posted to the forums Navigate to Navigation > CourseXXX > Participants Select a participant Navigate to Navigation > CourseXXX > Participants > ParticipantXXX > Forum posts > Posts Navigate to Navigation > CourseXXX > Participants > ParticipantXXX > Forum posts > Discussions I checked in the tables mentioned and found two contained fields with the data type = ntext. mdl_forum_posts - column name - message mdl_forum - column name - intro I changed both of these to type nvarchar(max) and the report works correctly. This problem is reported a few other times in tracker and moodle.org forums in other areas. Each time it is recommended to make this change and it rectifies the problem. The question was asked why use ntext data type at all. I am not however a DB analyst so can't really comment.

Hide

Permalink

Michael de Raadt added a comment - 20/Sep/12 3:46 PM

I'm bumping this issue as it has been duplicated a number of times.

It shouldn't be too hard to fix.

Show

Michael de Raadt added a comment - 20/Sep/12 3:46 PM I'm bumping this issue as it has been duplicated a number of times. It shouldn't be too hard to fix.

Hide

Permalink

Sam Hemelryk added a comment - 04/Oct/12 11:27 AM

Ok, I've had a look at this one using Oracle and could reproduce similar issues there.
I've made a couple of adjustments to the structure of a couple of queries that were breaking under oracle and have pushed them up for peer-review now.

Unfortunately I don't have MSSQL server at the moment, my windows machine has fritzed itself.
If the peer-reviewer could please test that, or find someone in the office to that would be most appreciated.
It is an easy test, just follow the replication instructions.

Many thanks
Sam

Show

Sam Hemelryk added a comment - 04/Oct/12 11:27 AM Ok, I've had a look at this one using Oracle and could reproduce similar issues there. I've made a couple of adjustments to the structure of a couple of queries that were breaking under oracle and have pushed them up for peer-review now. Unfortunately I don't have MSSQL server at the moment, my windows machine has fritzed itself. If the peer-reviewer could please test that, or find someone in the office to that would be most appreciated. It is an easy test, just follow the replication instructions. Many thanks Sam

Hide

Permalink

Ankit Agarwal added a comment - 16/Oct/12 2:15 PM

Sorry Sam,
I didnt realize, it needed MSSQL. I have unassigned myself from the reviewer.
A couple of things that I noticed are:-

Why do we need a Distinct in existing query? should it be returning distinct results anyway?
Will it be better not to reuse the shortname "f" for forum table?

Thanks

Show

Ankit Agarwal added a comment - 16/Oct/12 2:15 PM Sorry Sam, I didnt realize, it needed MSSQL. I have unassigned myself from the reviewer. A couple of things that I noticed are:- Why do we need a Distinct in existing query? should it be returning distinct results anyway? Will it be better not to reuse the shortname "f" for forum table? Thanks

Hide

Permalink

Sam Hemelryk added a comment - 16/Oct/12 3:29 PM

Hi Ankit,

No probs I'll put it back up for peer-review now.
Regarding the 2 points:

DISTINCT was needed because we were selecting based upon either a discussion or a post but we were really only interested in the forum. By using distinct we could be sure the forum was returned once no matter how many discussions the user had started, or how many posts they had made.
The new query avoids it by building that as a sub-select using group by and then joining to the sub select.
Reusing a table name within the sub-select doesn't have any adverse consequences that I am aware of. We do it in several statements I think so hopefully it is fine. There's no preference to it that I am aware of either.

Many thanks
Sam

Show

Sam Hemelryk added a comment - 16/Oct/12 3:29 PM Hi Ankit, No probs I'll put it back up for peer-review now. Regarding the 2 points: DISTINCT was needed because we were selecting based upon either a discussion or a post but we were really only interested in the forum. By using distinct we could be sure the forum was returned once no matter how many discussions the user had started, or how many posts they had made. The new query avoids it by building that as a sub-select using group by and then joining to the sub select. Reusing a table name within the sub-select doesn't have any adverse consequences that I am aware of. We do it in several statements I think so hopefully it is fine. There's no preference to it that I am aware of either. Many thanks Sam

Hide

Permalink

Dan Poltawski added a comment - 21/Oct/12 11:57 PM

Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

Show

Dan Poltawski added a comment - 21/Oct/12 11:57 PM Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

Hide

Permalink

Dan Poltawski added a comment - 01/Nov/12 3:10 PM

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Show

Dan Poltawski added a comment - 01/Nov/12 3:10 PM The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao

Hide

Permalink

Dan Poltawski added a comment - 05/Nov/12 1:23 PM

I tested in mssql and its looking good.

Show

Dan Poltawski added a comment - 05/Nov/12 1:23 PM I tested in mssql and its looking good.

Hide

Permalink

Dan Poltawski added a comment - 05/Nov/12 1:29 PM

Thanks Sam, i've integrated this now. I added one commit to fix trailing whitespace.

Show

Dan Poltawski added a comment - 05/Nov/12 1:29 PM Thanks Sam, i've integrated this now. I added one commit to fix trailing whitespace.

Hide

Permalink

Michael de Raadt added a comment - 06/Nov/12 2:05 PM

Test result: Success!

Tested with MySQL, PostgreSQL, Oracle and MS SQL under master.

Also tested in MS SQL under 2.2 and 2.3.

Show

Michael de Raadt added a comment - 06/Nov/12 2:05 PM Test result: Success! Tested with MySQL, PostgreSQL, Oracle and MS SQL under master. Also tested in MS SQL under 2.2 and 2.3.

Hide

Permalink

Luis de Vasconcelos added a comment - 06/Nov/12 2:32 PM

Michael, for MSSQL are you using SQLSRV or FreeTDS?

Show

Luis de Vasconcelos added a comment - 06/Nov/12 2:32 PM Michael, for MSSQL are you using SQLSRV or FreeTDS?

Hide

Permalink

Dan Poltawski added a comment - 06/Nov/12 2:44 PM

(I was testing MSSQL in freedts, for the records)

Show

Dan Poltawski added a comment - 06/Nov/12 2:44 PM (I was testing MSSQL in freedts, for the records)

Hide

Permalink

Dan Poltawski added a comment - 06/Nov/12 2:45 PM

Note that I don't think it should mattter, Luis, it was the query executed on the server which was the problem.

Show

Dan Poltawski added a comment - 06/Nov/12 2:45 PM Note that I don't think it should mattter, Luis, it was the query executed on the server which was the problem.

Hide

Permalink

Michael de Raadt added a comment - 06/Nov/12 4:03 PM

I tested this natively with SQL Server, so I think we have this covered.

Show

Michael de Raadt added a comment - 06/Nov/12 4:03 PM I tested this natively with SQL Server, so I think we have this covered.

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 10/Nov/12 1:11 AM

Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

(not really)

Closing, thanks!

Show

Eloy Lafuente (stronk7) added a comment - 10/Nov/12 1:11 AM Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

Hide

Permalink

Leo Furze-Waddock added a comment - 22/Nov/12 1:01 AM

People

Assignee:

Sam Hemelryk

Reporter:

Fred Weiss

Integrator:

Dan Poltawski

Tester:

Michael de Raadt

Participants:

Ankit Agarwal, Dan Poltawski, Eloy Lafuente (stronk7), Fred Weiss, Leo Furze-Waddock, Luis de Vasconcelos, Michael de Raadt, Sam Hemelryk

Vote (0)

Watch (4)

Dates

Created:

25/Jul/12 7:14 AM

Updated:

22/Nov/12 1:01 AM

Resolved:

10/Nov/12 1:11 AM

Agile

View on Board