Moodle
  1. Moodle
  2. MDL-5468

Race condition with quiz uniqueid getting the id from $CFG->attemptuniqueid

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.6
    • Fix Version/s: None
    • Component/s: Quiz
    • Labels:
      None
    • Environment:
      All
    • Database:
      Any
    • Affected Branches:
      MOODLE_16_STABLE
    • Rank:
      7803

      Description

      There is a possible race condition on the function call question_new_attempt_uniqueid since its getting its value from $CFG->attemptuniqueid + 1. This does not gurantee uniqueness.

      Here are the relevant commits:

      http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=d115d8c736ff44bfc959c2c7b5908e83fb00bc9f

      Look at question_new_attemp_uniqueid on lib/questionlib.php which was from mod/quiz/locallib.php

        Activity

        Hide
        Martin Dougiamas added a comment -

        From Martin Langhoff (martin at catalyst.net.nz) Wednesday, 10 May 2006, 01:26 PM:

        Gustav – the issue we have at the moment is that we don't quite understand how this additional unique id is meant to be used.

        From Martin Dougiamas (martin at moodle.com) Wednesday, 10 May 2006, 01:39 PM:

        Gustav, we can help fix/revert this but we do need to know ASAP why you thought it was necessary.

        I would have thought the id field in that table was unique already (more so than one generated in PHP like uniqueid).

        From Martin Dougiamas (martin at moodle.com) Wednesday, 10 May 2006, 02:15 PM:

        From Gustav's post on mantis:

        <blockquote>The next task now is to deal with the $attempt->id which will no longer be a unique identifier of an attempt once quiz and lesson both have their own attempts tables. $attempt->id will have to be replaced by a new $attempt->uniqueid. The simplest way I can think of for creating such a unique id each time either a quiz attempt or a lesson attempt or some other attempt is created is to use an entry in the config table that holds the next available id and is incremented if a new attempt is created.</blockquote>

        From Martin Langhoff (martin at catalyst.net.nz) Wednesday, 10 May 2006, 02:34 PM:

        Yup, I read that earlier, but it does not make much sense to me.

        My best guess is that the table is tracking relations to several external attempts table – in that case, the safe thing to do would be to make it a compound thing with two fields: attemptid, attempttable, where the pair is unique.

        But the logic of it all eludes me at the moment...

        From Gustav Delius (gwd2 at york.ac.uk) Wednesday, 10 May 2006, 03:31 PM:

        Ah, I had indeed failed to think about what would happen if two students started an attempt simultaneously.

        There are three ways to deal with this:

        1) Introduce a new table question_attempts with fields id, moduleid, attemptid and to create a new entry in this table each time a new uniqueid is needed. This will be a very quick change because only the function question_new_attempt_uniqueid() needs to be changed.

        2) In all tables that store the uniqueid field store instead both the moduleid and the attemptid.

        3) The proper way: Introduce a new table question_attempts to hold all the data about attemtps that all modules need to provide and then allow individual modules to extend this table with their own.

        I don't like 2). I like 3) but it is a major change and not to be recommended for the stable branch. So my proposal would be to do 1) first and then perhaps later (in HEAD) switch to 3).

        I appologize for not doing 3) in the first place, I was just a little lazy.

        If I don't hear better suggestions I will commit 1) tonight (provided I can get CVS to work for me again).

        From Jun Yamog (jun at catalyst.net.nz) Wednesday, 10 May 2006, 03:57 PM:

        1 seems to be ok to get a quick fix. Would this mean we will have a table like this?

        create table prefix_question_attempts (

        id serial primary key,

        moduleid integer,

        attemptid integer,

        unique (moduleid, attemptid)

        );

        Then we insert a new record on question_new_attempt_uniqueid, get the new returned id?

        I can help test the pgsql fix tomorrow.

        From Gustav Delius (gwd2 at york.ac.uk) Wednesday, 10 May 2006, 04:04 PM:

        That was indeed the idea. However I think I am going to do with just the two fields id and moduleid. The attemptid field is really of no use and is unnatural if one things of this new table as a first step towards solution 3) where the module's attempts table will be an extension of the question_attempts table.

        From Jun Yamog (jun at catalyst.net.nz) Friday, 12 May 2006, 01:11 PM:

        Please kindly review my fix here:

        http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=0006cdc8b33d08f2fb2626c977c6c4ea221d94b9

        From Martin Langhoff (martin at catalyst.net.nz) Friday, 12 May 2006, 01:57 PM:

        Discussing with Jun at the moment. Reworking things to have the table we are all keen on doesn't seem to be a good idea just before the release – too many bits of code are referring to this uniqueid field. So we've gone for a sequence in Pg, and a table with a single autoincrement field in MySQL (that acts effectively as a sequence).

        However, reading the patch, I am not 100% that the usage of insert_record() in question_new_attempt_uniqueid() (in the mysql case) is reliable.

        Even if it works with insert_record() today, it's not how insert_record() is meant to be used, and sanity fixes to insert_record() are very likely to break question_new_attempt_uniqueid().

        Perhaps we need a mysql_seq_nextval($seqname) in datalib that internally calls execute_sql()?

        From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 02:01 PM:

        Below is what I had just tried to post before Martin posted his reply. Luckily I got into the habit of copying what I wrote before posting on this bugtracker, in the past I always lost what I wrote if someone else had posted in the meantime. I am sending this off now and then think about what Martin wrote.

        Thanks Jun.

        I find that there is far too much explicit SQL in your code. So for example the question_new_attempt_uniqueid() function should simply use Moodle's insert_record() function.

        Can we please call the table question_attempts rather than quiz_attempts_uniqueids? It is not a quiz table at all.

        I think that the database update script should create entries in the table for all existing attempts. That will lead to much less confusion in the future and will make it easier to expand the role of the table.

        From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 02:03 PM:

        I am afraid that I don't understand Martin's post at all. Why is insert_record() not meant to be used for this? This appears to be exactly what it was designed for: to insert a new record and return its id. How could that possibly go wrong?

        From Jun Yamog (jun at catalyst.net.nz) Friday, 12 May 2006, 02:47 PM:

        Martin,

        I have done a quick test of it on mysql. I will do more testing with more data in. You are right that if it does work on insert_record, insert_record may change in the future if it checks for null objects.

        Gustav,

        I can rename the table for mysql and sequence for pgsql. I can insert the existing ids during the upgrade script rather than just getting the last id from mdl_config.

        From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 03:05 PM:

        Can someone explain to me what this stuff about insert_record() changing in the future is about? It doesn't sound likely that such a core function is going to change away from its documented behaviour in the future.

        From Martin Langhoff (martin at catalyst.net.nz) Friday, 12 May 2006, 03:49 PM:

        Hi Gustav!

        We don't literally want a table here, we just want a sequence to keep track of the latest number in non-racey way. In Pg we can do it with a sequence, in MySQL we need a bit of a hollow table which could even have just one row at any given time.

        In terms of insert_record(), it expects an object, and it definitely gives a warning if it doesn't get it. Internally, it passes the object to the AdoDB insert() which also expects an object, and tries to see what 'fields' the object has to build up the insert.

        We are not matching those assumptions at all, so if the insert works now is by sheer coincidence. A future 'bugfix' that makes insert_record() more strict will break this code. We should use explicit SQL here instead (which I know is ugly and unrelated – that's why I am suggesting to move it to a datalib function).

        From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 03:55 PM:

        Ah, yes of course, you should pass an object to insert_record().

        I don't think there is any reason to treat postgreSQL any different from mysql here. Simply use a proper table and insert_record() and it will work for all databases. This is a straightforward thing, no need to try to be clever.

        The main reason why I would like this to be a table from the outset is that I have plans to add more fields to the table in the future.

        From Martin Langhoff (martin at catalyst.net.nz) Friday, 12 May 2006, 04:09 PM:

        Hmm. Sequences are a standard thing – and it is not strange for an application to need them. Only that MySQL is a bit ackward about them.

        As far as I understand, you don't need a table right now to store more data (or rather... having a new table to store it would mean more changes just when we are trying to get 1.6 out of the door). To get 1.6 out without this problem, all we need is a reliable, non-racey way to get a uid. It probably makes sense to move data to a new table – agreed – but that's more of a 1.7 thing, maybe?

        So right now, if we can get a reliable uid, we are sorted, and what I am thinking is of adding a function to datalib that is something like get_sequence_next('sequencename') and hides the Pg/MySQL weirdness from unsuspecting passers-by...

        Do you think it makes sense?

        From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 04:13 PM:

        Hi Martin, I don't think it makes any sense to add new core library functions at this late stage given that what we need to do can be so easily be achieved with a table and the existing insert_record() function.

        From Martin Langhoff (martin at catalyst.net.nz) Friday, 12 May 2006, 04:20 PM:

        Gustav,

        Using insert_record() in this case is not reliable. Really. Look into what AdoDB does with it – it looks up the table, does an empty select, tries to read the fields from the table vs what the object has, etc, etc. Right now it works with warnings, but it is brittle and any trivial attempt at making that codepath tighter will break this.

        The minimum impact right now is to add

        • a 'sequence-tracking' table in mysql
        • a sequence in pg
        • code to fetch the uid from the sequence - which you are right isn't in its right place in the middle of questionlib

        We are still working not-on-cvs so we can rework this.

        From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 04:34 PM:

        Discussing this is beginning to be more effort than doing it

        I think we are probably talking at cross purposes here. You are probably just saying that the way Jun is using insert_record() is not proper. I agree with that.

        The easiest is probably if I give you the function the way I think it should be written:

        function question_new_attempt_uniqueid($modulename='quiz') {

        $attempt->modulename = $modulename;

        if (!id = insert_record('question_attempts', $attempt))

        { error('Could not create new entry in question_attempts table'); }

        return $id;

        }

        The default for the $modulename argument is supplied here so that we don't have to change all the calls to the function in Moodle 1.6. In Moodle 1.7 we will remove the default again so that modules are forced to supply the argument.

        From Gustav Delius (gwd2 at york.ac.uk) Saturday, 13 May 2006, 09:45 PM:

        If there is no objection to doing it my way then I'll commit.

        From Jun Yamog (jun at catalyst.net.nz) Monday, 15 May 2006, 10:46 AM:

        Hi Gustav,

        I have taken a peek at code in CVS, it looks good. I think this is the 3 option you have discussed? I will revert my changes on our code repository, so I can test it a few days from now.

        From Martin Dougiamas (martin at moodle.com) Monday, 15 May 2006, 11:27 AM:

        Sorry guys, just caught up on this.

        About insert_record() we need to have a really good talk about it somewhere else.

        1) All the recordset functions added in 1.6 have really confused the API for some developers already, and I do not want to continue this problem further with more arbitrary new functions until we have a really good plan mapped out.

        2) Moodle 1.7 will be supporting Oracle, MS SQL, Interbase etc as well, and we will need to clean up ALL the explicit SQL buried around, so please stick to core functions.

        From Eloy Lafuente (stronk7 at moodle.org) Thursday, 8 June 2006, 01:52 AM:

        While I was sorting MDL-5717 , I've seen one reference in the upgrade script to this bug.

        If I'm not wrong you are using the new prefix_question_attempts as one unique attemptid generator to use it in different modules (quiz, lesson...).

        Apart from discussing if this is a good technique or no (I've some doubts about it) I just wanted to call you about the upgrade script itself and how things are after executing it:

        1.- Under some databases (MSSQL) SERIAL fields aren't updateable by definition (no problem because we haven't MSSQL DBs yet).

        2.- Under some databases (Oracle) the auto-insert of the SERIAL is handled by one trigger that will overwrite the id field with its own sequence values (no problem because we haven't Oracle DBs yet). What about PostgreSQL. Does it use this trigger mechanism (I hope no or then we are broken now).

        3.- On some DB relying on sequences to do the job for id fields, using INSERT... (id) ... SELECT statements is a bad idea because the sequence can become inaccurate (not updated), pointing to some values that have been inserted manually. Then, when the next record is inserted, the PK of the table will cry. Is this PostgreSQL situation? Uhm... it depends about how it handle sequences but could be a problem

        So, independently of the method used to build such unique values (sequence, help table...) I recommend to avoid the INSERT ... (id) ... SELECT statements at all, relying EXCLUSIVELY in the insert_record() function to do the job. No matter if it has to iterate to insert thousands of records. I really think it's the only one way to keep all those DBs working under Moodle.

        Just one opinion, ciao

        From (penny at catalyst.net.nz) Thursday, 8 June 2006, 03:25 AM:

        I agree that insert_record should always be used.

        In cases it absolutely cannot be used, the only way to not break postgres is to do

        insert into table (field1, field2) values (value1, value2) where the primary key field isn't in the field list at all. That forces the default value for the pk field, which is nextval(sequence).

        If you insert your own value into a pk field without using the sequence, then you will get the sequence out of sequence and subsequent inserts using insert_record or the second option will fail.

        From Martin Langhoff (martin at catalyst.net.nz) Thursday, 8 June 2006, 04:20 AM:

        Then the fix is simple: the secondary tables should have their own id field (a true SERIAL), and have a questionattemptid field pointing to prefix_question_attempt.id ...

        Is that what everybody is thinking?

        From Jun Yamog (jun at catalyst.net.nz) Thursday, 8 June 2006, 06:00 AM:

        I just looked at mod/quiz/db/postgres7.php, without really testing anything. I see that there are 2 issues:

        1. The postgresql cleanup that I did wasn't tested with gustav changes. I reverted the fixes for MDL-5468 in my repository as it was a different issue after gustav went for fix #3. MartinL merged my code after gustav changes, which I think is incorrect. This would mean gustav's upgrade script will not work since uniqueid does not yet exist on quiz_attempts. My assumption was MartinL will merge my changes before gustav changes, as my upgrade script duplicates what was in mysql (pre bug fix 5468, duplicating the bug) so gustav's upgrade script will work. We will try to fix this.

        2. Inserting to postgresql and overriding with serials are allowed. What you have to do is to set the sequence during the upgrade, similar the upgrade script I have done with the rejected patch. So we have to issue ALTER SEQUENCE prefix_question_attempts_id_seq RESTART WITH $correctvalue. Although I guess it may not be an issue as there was no uniqueid for postgresql, but this assumption may not apply currently since the upgrade scripts have been issued. I will discuss with MartinL on how we will fix the pg side, which maybe similar to MDL-5717

        I don't have much opinion regarding the use of insert_record. However based from experience in supporting both postgresql and oracle, serials are not used for the unique ids. The usage of a sequence and a trigger provided closer semantics between postgresql and oracle, as stated above by eloy on #2.

        From Jun Yamog (jun at catalyst.net.nz) Thursday, 8 June 2006, 11:22 AM:

        I have attached here a partial fix for postgresql. Essentially this covers for the partially failed upgrade script of 2006051300. The situation of:

        1. One has upgraded to 2006051700, 2006051300 did not run fully complete since it will not see the unqiueid in quiz_attempts which was put into 2006051700.

        2. No new quizes attempted since the failed upgrade in #1. These means question_attempts is still empty.

        I have put a placeholder on the code for the complex situation of, question_attempts id went ahead of quiz_attempts id. Someone with deep knowledge of quiz/questions lib must resync them.

        Show
        Martin Dougiamas added a comment - From Martin Langhoff (martin at catalyst.net.nz) Wednesday, 10 May 2006, 01:26 PM: Gustav – the issue we have at the moment is that we don't quite understand how this additional unique id is meant to be used. From Martin Dougiamas (martin at moodle.com) Wednesday, 10 May 2006, 01:39 PM: Gustav, we can help fix/revert this but we do need to know ASAP why you thought it was necessary. I would have thought the id field in that table was unique already (more so than one generated in PHP like uniqueid). From Martin Dougiamas (martin at moodle.com) Wednesday, 10 May 2006, 02:15 PM: From Gustav's post on mantis: <blockquote>The next task now is to deal with the $attempt->id which will no longer be a unique identifier of an attempt once quiz and lesson both have their own attempts tables. $attempt->id will have to be replaced by a new $attempt->uniqueid. The simplest way I can think of for creating such a unique id each time either a quiz attempt or a lesson attempt or some other attempt is created is to use an entry in the config table that holds the next available id and is incremented if a new attempt is created.</blockquote> From Martin Langhoff (martin at catalyst.net.nz) Wednesday, 10 May 2006, 02:34 PM: Yup, I read that earlier, but it does not make much sense to me. My best guess is that the table is tracking relations to several external attempts table – in that case, the safe thing to do would be to make it a compound thing with two fields: attemptid, attempttable, where the pair is unique. But the logic of it all eludes me at the moment... From Gustav Delius (gwd2 at york.ac.uk) Wednesday, 10 May 2006, 03:31 PM: Ah, I had indeed failed to think about what would happen if two students started an attempt simultaneously. There are three ways to deal with this: 1) Introduce a new table question_attempts with fields id, moduleid, attemptid and to create a new entry in this table each time a new uniqueid is needed. This will be a very quick change because only the function question_new_attempt_uniqueid() needs to be changed. 2) In all tables that store the uniqueid field store instead both the moduleid and the attemptid. 3) The proper way: Introduce a new table question_attempts to hold all the data about attemtps that all modules need to provide and then allow individual modules to extend this table with their own. I don't like 2). I like 3) but it is a major change and not to be recommended for the stable branch. So my proposal would be to do 1) first and then perhaps later (in HEAD) switch to 3). I appologize for not doing 3) in the first place, I was just a little lazy. If I don't hear better suggestions I will commit 1) tonight (provided I can get CVS to work for me again). From Jun Yamog (jun at catalyst.net.nz) Wednesday, 10 May 2006, 03:57 PM: 1 seems to be ok to get a quick fix. Would this mean we will have a table like this? create table prefix_question_attempts ( id serial primary key, moduleid integer, attemptid integer, unique (moduleid, attemptid) ); Then we insert a new record on question_new_attempt_uniqueid, get the new returned id? I can help test the pgsql fix tomorrow. From Gustav Delius (gwd2 at york.ac.uk) Wednesday, 10 May 2006, 04:04 PM: That was indeed the idea. However I think I am going to do with just the two fields id and moduleid. The attemptid field is really of no use and is unnatural if one things of this new table as a first step towards solution 3) where the module's attempts table will be an extension of the question_attempts table. From Jun Yamog (jun at catalyst.net.nz) Friday, 12 May 2006, 01:11 PM: Please kindly review my fix here: http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=0006cdc8b33d08f2fb2626c977c6c4ea221d94b9 From Martin Langhoff (martin at catalyst.net.nz) Friday, 12 May 2006, 01:57 PM: Discussing with Jun at the moment. Reworking things to have the table we are all keen on doesn't seem to be a good idea just before the release – too many bits of code are referring to this uniqueid field. So we've gone for a sequence in Pg, and a table with a single autoincrement field in MySQL (that acts effectively as a sequence). However, reading the patch, I am not 100% that the usage of insert_record() in question_new_attempt_uniqueid() (in the mysql case) is reliable. Even if it works with insert_record() today, it's not how insert_record() is meant to be used, and sanity fixes to insert_record() are very likely to break question_new_attempt_uniqueid(). Perhaps we need a mysql_seq_nextval($seqname) in datalib that internally calls execute_sql()? From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 02:01 PM: Below is what I had just tried to post before Martin posted his reply. Luckily I got into the habit of copying what I wrote before posting on this bugtracker, in the past I always lost what I wrote if someone else had posted in the meantime. I am sending this off now and then think about what Martin wrote. Thanks Jun. I find that there is far too much explicit SQL in your code. So for example the question_new_attempt_uniqueid() function should simply use Moodle's insert_record() function. Can we please call the table question_attempts rather than quiz_attempts_uniqueids? It is not a quiz table at all. I think that the database update script should create entries in the table for all existing attempts. That will lead to much less confusion in the future and will make it easier to expand the role of the table. From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 02:03 PM: I am afraid that I don't understand Martin's post at all. Why is insert_record() not meant to be used for this? This appears to be exactly what it was designed for: to insert a new record and return its id. How could that possibly go wrong? From Jun Yamog (jun at catalyst.net.nz) Friday, 12 May 2006, 02:47 PM: Martin, I have done a quick test of it on mysql. I will do more testing with more data in. You are right that if it does work on insert_record, insert_record may change in the future if it checks for null objects. Gustav, I can rename the table for mysql and sequence for pgsql. I can insert the existing ids during the upgrade script rather than just getting the last id from mdl_config. From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 03:05 PM: Can someone explain to me what this stuff about insert_record() changing in the future is about? It doesn't sound likely that such a core function is going to change away from its documented behaviour in the future. From Martin Langhoff (martin at catalyst.net.nz) Friday, 12 May 2006, 03:49 PM: Hi Gustav! We don't literally want a table here, we just want a sequence to keep track of the latest number in non-racey way. In Pg we can do it with a sequence, in MySQL we need a bit of a hollow table which could even have just one row at any given time. In terms of insert_record(), it expects an object, and it definitely gives a warning if it doesn't get it. Internally, it passes the object to the AdoDB insert() which also expects an object, and tries to see what 'fields' the object has to build up the insert. We are not matching those assumptions at all, so if the insert works now is by sheer coincidence. A future 'bugfix' that makes insert_record() more strict will break this code. We should use explicit SQL here instead (which I know is ugly and unrelated – that's why I am suggesting to move it to a datalib function). From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 03:55 PM: Ah, yes of course, you should pass an object to insert_record(). I don't think there is any reason to treat postgreSQL any different from mysql here. Simply use a proper table and insert_record() and it will work for all databases. This is a straightforward thing, no need to try to be clever. The main reason why I would like this to be a table from the outset is that I have plans to add more fields to the table in the future. From Martin Langhoff (martin at catalyst.net.nz) Friday, 12 May 2006, 04:09 PM: Hmm. Sequences are a standard thing – and it is not strange for an application to need them. Only that MySQL is a bit ackward about them. As far as I understand, you don't need a table right now to store more data (or rather... having a new table to store it would mean more changes just when we are trying to get 1.6 out of the door). To get 1.6 out without this problem, all we need is a reliable, non-racey way to get a uid. It probably makes sense to move data to a new table – agreed – but that's more of a 1.7 thing, maybe? So right now, if we can get a reliable uid, we are sorted, and what I am thinking is of adding a function to datalib that is something like get_sequence_next('sequencename') and hides the Pg/MySQL weirdness from unsuspecting passers-by... Do you think it makes sense? From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 04:13 PM: Hi Martin, I don't think it makes any sense to add new core library functions at this late stage given that what we need to do can be so easily be achieved with a table and the existing insert_record() function. From Martin Langhoff (martin at catalyst.net.nz) Friday, 12 May 2006, 04:20 PM: Gustav, Using insert_record() in this case is not reliable. Really. Look into what AdoDB does with it – it looks up the table, does an empty select, tries to read the fields from the table vs what the object has, etc, etc. Right now it works with warnings, but it is brittle and any trivial attempt at making that codepath tighter will break this. The minimum impact right now is to add a 'sequence-tracking' table in mysql a sequence in pg code to fetch the uid from the sequence - which you are right isn't in its right place in the middle of questionlib We are still working not-on-cvs so we can rework this. From Gustav Delius (gwd2 at york.ac.uk) Friday, 12 May 2006, 04:34 PM: Discussing this is beginning to be more effort than doing it I think we are probably talking at cross purposes here. You are probably just saying that the way Jun is using insert_record() is not proper. I agree with that. The easiest is probably if I give you the function the way I think it should be written: function question_new_attempt_uniqueid($modulename='quiz') { $attempt->modulename = $modulename; if (!id = insert_record('question_attempts', $attempt)) { error('Could not create new entry in question_attempts table'); } return $id; } The default for the $modulename argument is supplied here so that we don't have to change all the calls to the function in Moodle 1.6. In Moodle 1.7 we will remove the default again so that modules are forced to supply the argument. From Gustav Delius (gwd2 at york.ac.uk) Saturday, 13 May 2006, 09:45 PM: If there is no objection to doing it my way then I'll commit. From Jun Yamog (jun at catalyst.net.nz) Monday, 15 May 2006, 10:46 AM: Hi Gustav, I have taken a peek at code in CVS, it looks good. I think this is the 3 option you have discussed? I will revert my changes on our code repository, so I can test it a few days from now. From Martin Dougiamas (martin at moodle.com) Monday, 15 May 2006, 11:27 AM: Sorry guys, just caught up on this. About insert_record() we need to have a really good talk about it somewhere else. 1) All the recordset functions added in 1.6 have really confused the API for some developers already, and I do not want to continue this problem further with more arbitrary new functions until we have a really good plan mapped out. 2) Moodle 1.7 will be supporting Oracle, MS SQL, Interbase etc as well, and we will need to clean up ALL the explicit SQL buried around, so please stick to core functions. From Eloy Lafuente (stronk7 at moodle.org) Thursday, 8 June 2006, 01:52 AM: While I was sorting MDL-5717 , I've seen one reference in the upgrade script to this bug. If I'm not wrong you are using the new prefix_question_attempts as one unique attemptid generator to use it in different modules (quiz, lesson...). Apart from discussing if this is a good technique or no (I've some doubts about it) I just wanted to call you about the upgrade script itself and how things are after executing it: 1.- Under some databases (MSSQL) SERIAL fields aren't updateable by definition (no problem because we haven't MSSQL DBs yet). 2.- Under some databases (Oracle) the auto-insert of the SERIAL is handled by one trigger that will overwrite the id field with its own sequence values (no problem because we haven't Oracle DBs yet). What about PostgreSQL. Does it use this trigger mechanism (I hope no or then we are broken now). 3.- On some DB relying on sequences to do the job for id fields, using INSERT... (id) ... SELECT statements is a bad idea because the sequence can become inaccurate (not updated), pointing to some values that have been inserted manually. Then, when the next record is inserted, the PK of the table will cry. Is this PostgreSQL situation? Uhm... it depends about how it handle sequences but could be a problem So, independently of the method used to build such unique values (sequence, help table...) I recommend to avoid the INSERT ... (id) ... SELECT statements at all, relying EXCLUSIVELY in the insert_record() function to do the job. No matter if it has to iterate to insert thousands of records. I really think it's the only one way to keep all those DBs working under Moodle. Just one opinion, ciao From (penny at catalyst.net.nz) Thursday, 8 June 2006, 03:25 AM: I agree that insert_record should always be used. In cases it absolutely cannot be used, the only way to not break postgres is to do insert into table (field1, field2) values (value1, value2) where the primary key field isn't in the field list at all. That forces the default value for the pk field, which is nextval(sequence). If you insert your own value into a pk field without using the sequence, then you will get the sequence out of sequence and subsequent inserts using insert_record or the second option will fail. From Martin Langhoff (martin at catalyst.net.nz) Thursday, 8 June 2006, 04:20 AM: Then the fix is simple: the secondary tables should have their own id field (a true SERIAL), and have a questionattemptid field pointing to prefix_question_attempt.id ... Is that what everybody is thinking? From Jun Yamog (jun at catalyst.net.nz) Thursday, 8 June 2006, 06:00 AM: I just looked at mod/quiz/db/postgres7.php, without really testing anything. I see that there are 2 issues: 1. The postgresql cleanup that I did wasn't tested with gustav changes. I reverted the fixes for MDL-5468 in my repository as it was a different issue after gustav went for fix #3. MartinL merged my code after gustav changes, which I think is incorrect. This would mean gustav's upgrade script will not work since uniqueid does not yet exist on quiz_attempts. My assumption was MartinL will merge my changes before gustav changes, as my upgrade script duplicates what was in mysql (pre bug fix 5468, duplicating the bug) so gustav's upgrade script will work. We will try to fix this. 2. Inserting to postgresql and overriding with serials are allowed. What you have to do is to set the sequence during the upgrade, similar the upgrade script I have done with the rejected patch. So we have to issue ALTER SEQUENCE prefix_question_attempts_id_seq RESTART WITH $correctvalue. Although I guess it may not be an issue as there was no uniqueid for postgresql, but this assumption may not apply currently since the upgrade scripts have been issued. I will discuss with MartinL on how we will fix the pg side, which maybe similar to MDL-5717 I don't have much opinion regarding the use of insert_record. However based from experience in supporting both postgresql and oracle, serials are not used for the unique ids. The usage of a sequence and a trigger provided closer semantics between postgresql and oracle, as stated above by eloy on #2. From Jun Yamog (jun at catalyst.net.nz) Thursday, 8 June 2006, 11:22 AM: I have attached here a partial fix for postgresql. Essentially this covers for the partially failed upgrade script of 2006051300. The situation of: 1. One has upgraded to 2006051700, 2006051300 did not run fully complete since it will not see the unqiueid in quiz_attempts which was put into 2006051700. 2. No new quizes attempted since the failed upgrade in #1. These means question_attempts is still empty. I have put a placeholder on the code for the complex situation of, question_attempts id went ahead of quiz_attempts id. Someone with deep knowledge of quiz/questions lib must resync them.
        Hide
        Michael Blake added a comment -

        assign to a valid user

        Show
        Michael Blake added a comment - assign to a valid user
        Hide
        Colin Donohue added a comment -

        This link is not working so i cannot get the solution:

        http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=0006cdc8b33d08f2fb2626c977c6c4ea221d94b9

        Can anyone help?

        Show
        Colin Donohue added a comment - This link is not working so i cannot get the solution: http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=0006cdc8b33d08f2fb2626c977c6c4ea221d94b9 Can anyone help?
        Hide
        Timothy Takemoto added a comment -

        Same problem here as Colin Donohue.

        But I am hopeful that the reason why I am having the problem is that while I was making a database backup students submitted quiz attempts.
        So if when I make a backup I ensure that there is no one using the system, I am hopeful that it should be okay.

        But it would be nice to know what was in
        http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=0006cdc8b33d08f2fb2626c977c6c4ea221d94b9

        Show
        Timothy Takemoto added a comment - Same problem here as Colin Donohue. But I am hopeful that the reason why I am having the problem is that while I was making a database backup students submitted quiz attempts. So if when I make a backup I ensure that there is no one using the system, I am hopeful that it should be okay. But it would be nice to know what was in http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=0006cdc8b33d08f2fb2626c977c6c4ea221d94b9

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: