Duplicate records in usr_custom_layout cause fatal crash when copying a collection

[Ed Cooper]

When creating a collection by copying a previous collection we are getting an error of:

Site unavailable
A nonrecoverable error occurred. This probably means you have encountered a bug in the system

PHP is reporting the following error and we are on the latest version of Mahara to date 15.10.0:

2015/11/13 09:02:51 [error] 1077#0: *7505 FastCGI sent in stderr: "PHP message: [WAR] 0a (lib/errors.php:747) get_record_sql found more than one row. If you meant to retrieve more than one record, use get_records_*, otherwise check your code or database for inconsistencies
PHP message: Call stack (most recent first):
PHP message: * log_message("get_record_sql found more than one row. If you mea...", 8, true, true) at /scratch/var/mahara/htdocs/lib/errors.php:97
PHP message: * log_warn("get_record_sql found more than one row. If you mea...") at /scratch/var/mahara/htdocs/lib/errors.php:747
PHP message: * SQLException->__construct("get_record_sql found more than one row. If you mea...") at /scratch/var/mahara/htdocs/lib/dml.php:328
PHP message: * get_record_sql("SELECT * FROM "usr_custom_layout" WHERE "layout" ...", array(size 2)) at /scratch/var/mahara/htdocs/lib/dml.php:291
PHP message: * get_record("usr_custom_layout", "layout", "203", "usr", "1770") at /scratch/var/mahara/htdocs/lib/view.php:428
PHP message: * View::create_from_template(array(size 5), "40765", "1770", true, false) at /scratch/var/mahara/htdocs/lib/collection.php:292
PHP message: * Collection::create_from_template(array(size 6), 4307) at /scratch/var/mahara/htdocs/lib/view.php:6376
PHP message: * createview_submit(object(Pieform), array(size 7)) at Unknown:0
PHP message: * call_user_func_array("createview_submit", array(size 2)) at /scratch/var/mahara/htdocs/lib/pieforms/pieform.php:537
PHP message: * Pieform->__construct(array(size 8)) at /scratch/var/mahara/htdocs/lib/pieforms/pieform.php:164
PHP message: * Pieform::process(array(size 8)) at /scratch/var/mahara/htdocs/lib/pieforms/pieform.php:71
PHP message: * pieform(array(size 8)) at /scratch/var/mahara/htdocs/view/choosetemplate.php:27
PHP message:
PHP message: [WAR] 0a (lib/dml.php:327) get_record_sql found more than one row. If you meant to retrieve more than one record, use get_records_*, otherwise check your code or database for inconsistencies
PHP message: Ca

Cheers,
Ed

[Robert Lyon (robertl-9)]

Hi Ed,

This error is due to your database having duplicate rows in the usr_custom_layout table for the user before they try to copy the collection, of which at least one page has the same layout.

The code is expecting to retrieve only one row to match against the current page being copied.

A few users of mahara have reported this problem but I've yet not been able to replicate how they got duplicate rows in the usr_custom_layout table in the first place.

If you could shed some light on that it would be great.

Anyway a fix for the problem is to remove by first finding them:

 SELECT * FROM usr_custom_layout WHERE usr IS NOT NULL AND usr > 0 ORDER BY usr, layout;

The problem rows should show the same 'usr' id and 'layout' id in pairs.

You just need to pick one of the pair and delete id

Cheers

Robert

[Aaron Wells (u-aaronw)]

Hi Ed,

Thanks for the bug report!

Hm, I think the problem here is that our code assumes there will be no more than one record in usr_custom_layout for each (user, layout) pair, or (group, layout), or (institution, layout). But, there's no uniqueness constraint for that in the database. Our insert code is trying to make sure not to insert duplicate records, but apparently it can happen in some anomalous circumstances (maybe a bug in the code, or a race condition).

So our fix should be:

1. Put a uniqueness constraint on those relations, if possible. (And when applying that uniqueness constraint in the upgrade block, make sure to delete any duplicate records first.)

2. If we can't put a uniqueness constraint on those relations, change this bit of code to tolerate the presence of more than one record. Maybe we should add an optional parameter to the get_record() family of arguments to tell it whether it should error out if it finds more than one record?

Ed,

In the meantime, for your specific site, the workaround is to delete the duplicate records from the database, which you should be able to do by running this query:

DELETE FROM "usr_custom_layout" WHERE "id" NOT IN (SELECT MIN("id") FROM "usr_custom_layout" GROUP BY "usr", "group", "institution", "layout");

Or if you want to take a more cautious approach, you can use this query to view the duplicate records, and then delete all but one from each set, individually:

SELECT ucl.*
FROM "usr_custom_layout" ucl
INNER JOIN ( SELECT "usr", "group", "institution", "layout" FROM "usr_custom_layout" GROUP BY "usr", "group", "institution", "layout" HAVING COUNT(*) > 1) q
ON (ucl.usr=q.usr OR ucl.group=q.group OR ucl.institution=q.institution) AND ucl.layout=q.layout
ORDER BY "usr", "group", "institution", "layout";

... if you're using MySQL you'll need to replace those "double quotes" with `backticks`, or run "SET SQL_MODE='POSTGRESQL';"

Cheers,
Aaron

[Robert Lyon (robertl-9)]

Actually a slightly better sql query to find the duplicated user custom layouts:

  SELECT * FROM usr_custom_layout WHERE usr IN (SELECT usr FROM usr_custom_layout WHERE usr IS NOT NULL GROUP BY usr, layout HAVING COUNT(*) > 1) ORDER BY usr, layout;

This will ony return the places where there are duplication

[Robert Lyon (robertl-9)]

I mean only return users that have duplication. Not all the layouts for those users will be duplicated but at least one will be.

[Robert Lyon (robertl-9)]

Will mark this bug as high as it is happening to multiple mahara sites and causes problems copying collections

[Mahara Bot (dev-mahara)]

Patch for "master" branch: https://reviews.mahara.org/5722

[Ed Cooper (ed-cooper)]

Thanks for the update.

I've tried running the delete statement:

DELETE FROM `usr_custom_layout` WHERE `id` NOT IN ( SELECT MIN(`id`) FROM `usr_custom_layout` GROUP BY `usr`, `group`, `institution`, `layout`);

But it results in:

ERROR 1093 (HY000): You can't specify target table 'usr_custom_layout' for update in FROM clause

[Ed Cooper (ed-cooper)]

Hi,

Sorry for bumping this, but I have a client wanting to use this feature desperately. Any idea why the delete query isn't working?

Thanks, Ed

[Aaron Wells (u-aaronw)]

Hi Ed,

Sorry, I guess in MySQL you can't run a delete query with the deleting table in a subquery like that. So you'll need to do it in two steps. First to locate the records and store them in a temp table, and then to delete them.

CREATE TEMPORARY TABLE dupes AS
SELECT id FROM usr_custom_layout WHERE id NOT IN (SELECT MIN(id) FROM usr_custom_layout GROUP BY usr, `group`, institution, layout;

DELETE FROM usr_custom_layout WHERE id IN (SELECT id FROM dupes);

Cheers,
Aaron

[Mahara Bot (dev-mahara)]

Patch for "master" branch: https://reviews.mahara.org/5764

[Aaron Wells (u-aaronw)]

There are currently two patches marked for this bug:

1. https://reviews.mahara.org/5722: Robert's earlier patch, which rewrites the usr_custom_layout insertion code to use ensure_record_exists in an attempt to use database transactions to avoid getting duplicate records. Unfortunately, this won't actually solve the problem, although it's still a good idea to merge it in because it's cleaner code.

2. https://reviews.mahara.org/5764: A new patch I just wrote, that makes our get_record() method print a warning when there are duplicate records, instead of crashing. This *should* resolve this problem.

[Ed Cooper (ed-cooper)]

That's done it, thanks.

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/5722
Committed: https://git.mahara.org/mahara/mahara/commit/5af4dea021a65f2b65e7a4069ce1f9065a112f54
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit 5af4dea021a65f2b65e7a4069ce1f9065a112f54
Author: Robert Lyon <email address hidden>
Date: Mon Nov 16 13:55:09 2015 +1300

Bug 1515929: Changing the insert of usr custom layouts

Rewrite to use ensure_record_exists instead of re-inventing
the wheel.

behatnotneeded: Covered by existing tests

Change-Id: Id57be0e00f14ab65dd40b4ef13f696e72f608d46
Signed-off-by: Robert Lyon <email address hidden>

[Mahara Bot (dev-mahara)]

Patch for "15.10_STABLE" branch: https://reviews.mahara.org/5779

[Mahara Bot (dev-mahara)]

Patch for "15.10_STABLE" branch: https://reviews.mahara.org/5788

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/5779
Committed: https://git.mahara.org/mahara/mahara/commit/ccae9cf94143c20d9aa2cc6ccc12b8a370569118
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.10_STABLE

commit ccae9cf94143c20d9aa2cc6ccc12b8a370569118
Author: Robert Lyon <email address hidden>
Date: Mon Nov 16 13:55:09 2015 +1300

Bug 1515929: Changing the insert of usr custom layouts

Rewrite to use ensure_record_exists instead of re-inventing
the wheel.

behatnotneeded: Covered by existing tests

Change-Id: Id57be0e00f14ab65dd40b4ef13f696e72f608d46
Signed-off-by: Robert Lyon <email address hidden>
(cherry picked from commit 5af4dea021a65f2b65e7a4069ce1f9065a112f54)

[Ed Cooper (ed-cooper)]

The issue seems to have reappeared for some admins when masquerading as a student and viewing a collection that a student has copied:

2015/11/26 12:55:39 [error] 1077#0: *121274 FastCGI sent in stderr: ""SELECT COUNT(*)
PHP message: FROM {view} v
PHP message: ...", array(size 24)) at /scratch/var/mahara/htdocs/lib/dml.php:276
PHP message: * count_records_sql("SELECT COUNT(*)
PHP message: FROM {view} v
PHP message: ...", array(size 24)) at /scratch/var/mahara/htdocs/lib/view.php:4303
PHP message: * View::view_search("streek", null, null, null, 10, 0, true, array(size 1), array(size 1), false, array(size 3), null, null, "969", true) at /scratch/var/mahara/htdocs/lib/view.php:4433
PHP message: * View::shared_to_user("streek", null, 10, 0, "lastchanged", "desc", array(size 3), "969") at /scratch/var/mahara/htdocs/view/sharedviews.php:151
PHP message:" while reading response header from upstream, client: ***, server: _, request: "POST /view/sharedviews.php HTTP/1.1", upstream: "fastcgi://unix:/tmp/php-fpm.sock:", host: "***", referrer: "***"

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/5788
Committed: https://git.mahara.org/mahara/mahara/commit/32f270451cf9b885425569470a87e71bcf7f4142
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.10_STABLE

commit 32f270451cf9b885425569470a87e71bcf7f4142
Author: Aaron Wells <email address hidden>
Date: Tue Nov 24 16:53:16 2015 +1300

Make get_record warn instead of dying, by default

Bug 1515929: Usually when we use get_record(), we're
querying against a record that has a uniqueness constraint
guaranteeing that it is unique, in which case the PHP
code that dies on non-uniqueness is redundant.

In the remaining cases, we're dealing with records
that for some reason can't have a uniqueness constraint,
and the dying just causes the site to entirely stop
working, when it would be more useful to have it continue
to work but throw a warning message to the logs.

behatnotneeded: Covered by existing test cases

Change-Id: I264f72e3a8904293d78909410f68b29f2c78db3c

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/5764
Committed: https://git.mahara.org/mahara/mahara/commit/bc354483a91c59becf695630b93946ea5bf49579
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit bc354483a91c59becf695630b93946ea5bf49579
Author: Aaron Wells <email address hidden>
Date: Tue Nov 24 16:53:16 2015 +1300

Make get_record warn instead of dying, by default

Bug 1515929: Usually when we use get_record(), we're
querying against a record that has a uniqueness constraint
guaranteeing that it is unique, in which case the PHP
code that dies on non-uniqueness is redundant.

In the remaining cases, we're dealing with records
that for some reason can't have a uniqueness constraint,
and the dying just causes the site to entirely stop
working, when it would be more useful to have it continue
to work but throw a warning message to the logs.

behatnotneeded: Covered by existing test cases

Change-Id: I264f72e3a8904293d78909410f68b29f2c78db3c

[Mahara Bot (dev-mahara)]

Patch for "15.04_STABLE" branch: https://reviews.mahara.org/5807

[Mahara Bot (dev-mahara)]

Patch for "1.10_STABLE" branch: https://reviews.mahara.org/5808

[Aaron Wells (u-aaronw)]

We probably should have backported this to 15.04 and 1.10 as well, because I suspect it affects all Mahara versions since we added the flexible layout feature in 1.8.

I've cherry-picked it back to those versions now, so we can include it in the next release for them.

[Ed Cooper (ed-cooper)]

Sorry to bump this, can you check response #17 https://bugs.launchpad.net/mahara/+bug/1515929/comments/17

[Aaron Wells (u-aaronw)]

Hi Ed,

That looks like an unrelated error. It's going through the "get_recordset_sql()" method, which doesn't care about duplicate records. And based on the line of code mentioned in that stacktrace, I believe that particular query is a "select count(*)" query with no "group by" clause, which means it's by definition impossible for it to return more than one row anyhow.

In fact, that error stack doesn't seem to indicate any specific error message from PHP/Mahara at all. There's just the FastCGI error, the SQL query, and a PHP backtrace. This might be a longshot, but is it maybe an "echo" statement that one your developers placed into your code, to print out what the query was at that particular spot?

If you're still encountering the usr_custom_layout duplicate record errors, have you tried patch https://reviews.mahara.org/5788 yet (or upgraded to 15.04.1)?

Cheers,
Aaron

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/5807
Committed: https://git.mahara.org/mahara/mahara/commit/dab8eed23fb42cc135644c66f7ef865fae811dc9
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.04_STABLE

commit dab8eed23fb42cc135644c66f7ef865fae811dc9
Author: Aaron Wells <email address hidden>
Date: Tue Nov 24 16:53:16 2015 +1300

Make get_record warn instead of dying, by default

Bug 1515929: Usually when we use get_record(), we're
querying against a record that has a uniqueness constraint
guaranteeing that it is unique, in which case the PHP
code that dies on non-uniqueness is redundant.

In the remaining cases, we're dealing with records
that for some reason can't have a uniqueness constraint,
and the dying just causes the site to entirely stop
working, when it would be more useful to have it continue
to work but throw a warning message to the logs.

behatnotneeded: Covered by existing test cases

Change-Id: I264f72e3a8904293d78909410f68b29f2c78db3c
(cherry picked from commit 32f270451cf9b885425569470a87e71bcf7f4142)

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/5808
Committed: https://git.mahara.org/mahara/mahara/commit/59b558462114ea0af95b7e530779fd324406377b
Submitter: Robert Lyon (<email address hidden>)
Branch: 1.10_STABLE

commit 59b558462114ea0af95b7e530779fd324406377b
Author: Aaron Wells <email address hidden>
Date: Tue Nov 24 16:53:16 2015 +1300

Make get_record warn instead of dying, by default

Bug 1515929: Usually when we use get_record(), we're
querying against a record that has a uniqueness constraint
guaranteeing that it is unique, in which case the PHP
code that dies on non-uniqueness is redundant.

In the remaining cases, we're dealing with records
that for some reason can't have a uniqueness constraint,
and the dying just causes the site to entirely stop
working, when it would be more useful to have it continue
to work but throw a warning message to the logs.

behatnotneeded: Covered by existing test cases

Change-Id: I264f72e3a8904293d78909410f68b29f2c78db3c