Last modified: 2014-06-21 19:58:06 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T45475, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 43475 - ORMRow::insert() fails on PostgreSQL
ORMRow::insert() fails on PostgreSQL
Status: NEW
Product: MediaWiki
Classification: Unclassified
Database (Other open bugs)
1.21.x
All All
: High major (vote)
: ---
Assigned To: Nobody - You can work on this!
:
: 43458 (view as bug list)
Depends on:
Blocks: postgres
  Show dependency treegraph
 
Reported: 2012-12-28 02:06 UTC by Tim Landscheidt
Modified: 2014-06-21 19:58 UTC (History)
12 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Tim Landscheidt 2012-12-28 02:06:54 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.
Comment 1 Tim Landscheidt 2012-12-28 05:27:23 UTC
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
Comment 2 Marcin Cieślak 2012-12-28 20:01:25 UTC
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?
Comment 3 Aude 2012-12-28 20:14:15 UTC
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.
Comment 4 Tim Landscheidt 2012-12-28 21:19:31 UTC
(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 :-).
Comment 5 Tim Landscheidt 2012-12-28 21:34:14 UTC
(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.
Comment 6 Aude 2012-12-28 22:54:09 UTC
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.
Comment 7 Andre Klapper 2013-03-20 15:48:48 UTC
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.
Comment 8 db [inactive,noenotif] 2013-03-26 20:40:21 UTC
Gerrit change #41020 merged, Gerrit change #41181 abandoned - bug maybe fixed
Comment 9 Andre Klapper 2013-04-09 20:04:00 UTC
Tim / Aude: Is this fixed, or is some more work needed? 
Not clear (see last comment)...
Comment 10 OverlordQ 2013-04-09 20:38:32 UTC
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.
Comment 11 db [inactive,noenotif] 2013-05-10 16:18:16 UTC
Found Gerrit change #58422, maybe fixed now?
Comment 12 Andre Klapper 2013-05-14 13:56:27 UTC
(In reply to comment #11)
> Found Gerrit change #58422, maybe fixed now?

overlordq: Ping - did your patch by any chance fix this bug report?
Comment 13 Andre Klapper 2013-05-29 12:34:47 UTC
(In reply to comment #11)
> Found Gerrit change #58422, maybe fixed now?

overlordq: Ping - did your patch by any chance fix this bug report?
Comment 14 Andre Klapper 2013-06-25 10:38:01 UTC
(In reply to comment #11)
> Found Gerrit change #58422, maybe fixed now?

overlordq: Ping - did your patch by any chance fix this bug report?
Comment 15 Marcin Cieślak 2013-12-06 13:18:21 UTC
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.
Comment 16 Marcin Cieślak 2013-12-06 14:39:22 UTC
*** Bug 43458 has been marked as a duplicate of this bug. ***
Comment 17 Marcin Cieślak 2013-12-07 22:27:55 UTC
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.
Comment 18 Marcin Cieślak 2013-12-07 22:31:01 UTC
(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.
Comment 21 Marcin Cieślak 2013-12-07 22:32:01 UTC
> ... bug 37061 ...

er, bug 37601...
Comment 22 Mark A. Hershberger 2014-06-21 19:58:06 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links