Last modified: 2014-06-21 19:58:06 UTC
As mentioned in bug 37601 comment #2 and showing up again when testing Gerrit change #27939, when inserting a new row, ORMRow explicitly passes NULL as the value for the ID column which in MySQL means "use next value", but in PostgreSQL means "use NULL". It can be worked around by passing the keyword "DEFAULT" (without quotes) as the value, e. g. "INSERT INTO t (c) VALUES (DEFAULT);". The easiest solution is to exclude ID columns in ORMRow::getWriteValues() when their value is null, but this prohibits the (not so probable) scenario where you set the id field of a row to null and then call update() to assign it a new id. However, the current code would fail in this case as well as it assumes via getUpdateConditions() that the value of the id field prior to the update is the same as after. I'll submit a changeset with that understanding later.
Gerrit change #41020, tested on MySQL, PostgreSQL (for the most part) and SQLite for: | tests/phpunit/includes/db/TestORMRowTest.php | tests/phpunit/includes/SiteConfigurationTest.php | tests/phpunit/includes/site/MediaWikiSiteTest.php | tests/phpunit/includes/site/SiteArrayTest.php | tests/phpunit/includes/site/SiteListTest.php | tests/phpunit/includes/site/SiteObjectTest.php | tests/phpunit/includes/site/SitesTest.php
I think that if someone aims at the "ORM" layer one should handle sequences properly. Your solution with using the default is correct given that table definition includes the default value out of appropriate sequence. gwtorm (Google Web Toolkit ORM used by Gerrit internally) takes a different approach - it uses sequences natively on PostgreSQL and emulates sequences on MySQL by creating additional one-field tables to retrieve the next ID from. Not sure if "id" as the field name is somehow guaranteed by this "ORM" thing?
an additional issue is that ORMRow uses Database::insertID() but mLatestId never gets set and is null. $this->setField( 'id', $dbw->insertId() ); Now, PostgreSQL supports ' RETURNING id' as part of the query. We could make 'returning' ( 'returning' => 'id_field_name' ) as an optional query parameter for DatabasePostgres::insert(). and then be able to set mLatestId when inserting a single row. (which ORMRow does, and likely elsewhere in the code) I need to poke more at the code to be sure this could work and that Tim's patch, which is part of the fix, also works okay with Wikibase and other places it's used.
(In reply to comment #2) > I think that if someone aims at the "ORM" layer one should handle sequences > properly. Your solution with using the default is correct given that table > definition includes the default value out of appropriate sequence. > gwtorm (Google Web Toolkit ORM used by Gerrit internally) takes a different > approach - it uses sequences natively on PostgreSQL and emulates sequences on > MySQL by creating additional one-field tables to retrieve the next ID from. > Not sure if "id" as the field name is somehow guaranteed by this "ORM" thing? Yes, it's hard-coded. Personally, I am a huge friend of leaving handling serial values to the DBMS (well, and an opponent of ORM in general :-)), so I think even in MySQL those should be generated by the DBMS. (In reply to comment #3) > an additional issue is that ORMRow uses Database::insertID() but mLatestId > never gets set and is null. > $this->setField( 'id', $dbw->insertId() ); > Now, PostgreSQL supports ' RETURNING id' as part of the query. We could make > 'returning' ( 'returning' => 'id_field_name' ) as an optional query parameter > for DatabasePostgres::insert(). > and then be able to set mLatestId when inserting a single row. (which ORMRow > does, and likely elsewhere in the code) > [...] You are right. AFAICS, we also need a test for this cycle (insert -> update -> read) as the current test doesn't seem to cover this. It would be nice if any solution could cover not just PostgreSQL (and MySQL) and the ORM system alone. Grepping for nextSequenceValue(), it seems to be used in most cases in conjunction with insert(), so integrating it in insert() might be possible (i. e., for PostgreSQL, use "RETURNING", for MySQL, use mysql_insert_id(), for other DBMS, first call nextSequenceValue(), then use that value), but there 28 instances of it to study :-).
(In reply to comment #3) > an additional issue is that ORMRow uses Database::insertID() but mLatestId > never gets set and is null. > $this->setField( 'id', $dbw->insertId() ); > Now, PostgreSQL supports ' RETURNING id' as part of the query. We could make > 'returning' ( 'returning' => 'id_field_name' ) as an optional query parameter > for DatabasePostgres::insert(). > and then be able to set mLatestId when inserting a single row. (which ORMRow > does, and likely elsewhere in the code) > [...] How about naming the option "insertid" to stay in theme and add another called "sequence"? If "insertid" is set to the name of a table attribute and the DBMS is one of those that support a "RETURNING" clause or have an equivalent to mysql_insert_id(), the query is executed and $this->mInsertId is set afterwards to the value returned. For other DBMS, $this->nextSequenceValue() is called with the name of the sequence, and the generated value is then passed as the table attribute named by "insertid", and if the query succeeds, $this->mInsertId is set to it as well.
https://gerrit.wikimedia.org/r/#/c/41181/ is an attempt to make setting the id work. Okay with naming the option "insertid" and other suggestions.
Note: As this has the 1.21.0 target milestone set, Aude's patch in Gerrit needs to receive a review / merge. If this does not receive a patch in the next weeks, the Target Milestone will likely get removed.
Gerrit change #41020 merged, Gerrit change #41181 abandoned - bug maybe fixed
Tim / Aude: Is this fixed, or is some more work needed? Not clear (see last comment)...
I tried running the PHPUnit tests but still running into lingering cases of: 12) ORMTableTest::testIgnoreErrorsOverride DatabasePostgres::reportQueryError: No transaction to rollback, something got out of sync! ala bug 44136.
Found Gerrit change #58422, maybe fixed now?
(In reply to comment #11) > Found Gerrit change #58422, maybe fixed now? overlordq: Ping - did your patch by any chance fix this bug report?
Gerrit change #58422 got merged, but that didn't help. After a bit of fighting with some other bugs I came to the following result: 1) TestORMRowTest::testSaveAndRemove with data set #0 (array('Foobar', '2012-01-01 02:02:02 GMT', 42, 9000.1, true, array(13, 11, 7, 5, 3, 2), stdClass), true) DBQueryError: A database error has occurred. Did you forget to run maintenance/update.php after upgrading? See: https://www.mediawiki.org/wiki/Manual:Upgrading#Run_the_update_script Query: INSERT INTO "unittest_orm_test" (test_id,test_name,test_age,test_height,test_awesome,test_stuff,test_moarstuff,test_time) VALUES (NULL,'Foobar','42','9000.1',1,'a:6:{i:0;i:13;i:1;i:11;i:2;i:7;i:3;i:5;i:4;i:3;i:5;i:2;}','O:8:"stdClass":3:{s:3:"foo";s:3:"bar";s:3:"bar";a:2:{i:0;i:4;i:1;i:2;}s:3:"baz";b:1;}','2012-01-01 02:02:02 GMT') Function: ORMTable::insertRow Error: 23502 ERROR: null value in column "test_id" violates not-null constraint /usr/home/saper/test/mytest/includes/db/Database.php:1111 /usr/home/saper/test/mytest/includes/db/DatabasePostgres.php:511 /usr/home/saper/test/mytest/includes/db/Database.php:1077 /usr/home/saper/test/mytest/includes/db/DatabasePostgres.php:871 /usr/home/saper/test/mytest/includes/db/ORMTable.php:1015 /usr/home/saper/test/mytest/includes/db/ORMRow.php:352 /usr/home/saper/test/mytest/tests/phpunit/includes/db/ORMRowTest.php:143 /usr/home/saper/test/mytest/tests/phpunit/MediaWikiTestCase.php:123 /usr/home/saper/test/mytest/tests/phpunit/MediaWikiPHPUnitCommand.php:80 /usr/home/saper/test/mytest/tests/phpunit/MediaWikiPHPUnitCommand.php:64 /usr/home/saper/test/mytest/tests/phpunit/phpunit.php:119 Still inserting NULL in the SERIAL field is wrong: minitest=# create table a (id serial primary key, s int); NOTICE: CREATE TABLE will create implicit sequence "a_id_seq" for serial column "a.id" NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "a_pkey" for table "a" CREATE TABLE minitest=# insert into a (id,s) values (null, 2); ERROR: null value in column "id" violates not-null constraint Can't we come up with proper handling of sequences/ids? Nobody said writing "ORM" is easy... Reseting back to NEW as the change merged does not solve the problem. This causes SiteSQLStoreTest::testSaveSites to fail, among others.
*** Bug 43458 has been marked as a duplicate of this bug. ***
Sorry for talking to myself, but the abovementioned fix repairs test failures I've had with SiteSQLStore, so I kind of believe the fix is kind of good, but the tests are broken.
(In reply to comment #17) > Sorry for talking to myself, but the abovementioned fix repairs test failures > I've had with SiteSQLStore, so I kind of believe the fix is kind of good, but > the tests are broken. Sorry this was meant for bug 37061. But it seems that gerrit change I5d68ce327261f897a774993c165e8161e654b2c6 might actually fix insertRow for PostgreSQL.
> ... bug 37061 ... er, bug 37601...
Removing target milestone that was in the past. If you want this in a specific release, have a good reason AND you are willing to find resources to fix this bug, feel free to change it to something appropriate.