Page MenuHomePhabricator

ORMRow::insert() fails on PostgreSQL
Closed, InvalidPublic

Description

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.


Version: 1.21.x
Severity: major
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=37601

Details

Reference
bz43475

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusAssignedTask
InvalidNone
InvalidNone

Event Timeline

bzimport raised the priority of this task from to High.
bzimport set Reference to bz43475.
bzimport added a subscriber: Unknown Object (MLST).
scfc created this task.Dec 28 2012, 2:06 AM
scfc added a comment.Dec 28 2012, 5:27 AM

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
saper added a comment.Dec 28 2012, 8:01 PM

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?

aude added a comment.Dec 28 2012, 8:14 PM

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.

scfc added a comment.Dec 28 2012, 9:19 PM

(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 :-).

scfc added a comment.Dec 28 2012, 9:34 PM

(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.

aude added a comment.Dec 28 2012, 10:54 PM

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 41020 merged, gerrit 41181 abandoned - bug maybe fixed

Tim / Aude: Is this fixed, or is some more work needed?
Not clear (see last comment)...

overlordq wrote:

I tried running the PHPUnit tests but still running into lingering cases of:

  1. ORMTableTest::testIgnoreErrorsOverride

DatabasePostgres::reportQueryError: No transaction to rollback, something got out of sync!

ala bug 44136.

Found gerrit 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?

(In reply to comment #11)

Found Gerrit change #58422, maybe fixed now?

overlordq: Ping - did your patch by any chance fix this bug report?

(In reply to comment #11)

Found Gerrit change #58422, maybe fixed now?

overlordq: Ping - did your patch by any chance fix this bug report?

saper added a comment.Dec 6 2013, 1:18 PM

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.

saper added a comment.Dec 6 2013, 2:39 PM
  • Bug 43458 has been marked as a duplicate of this bug. ***
saper added a comment.Dec 7 2013, 10:27 PM

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.

saper added a comment.Dec 7 2013, 10:31 PM

(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.

saper added a comment.Dec 7 2013, 10:31 PM

content hidden as private in Bugzilla

saper added a comment.Dec 7 2013, 10:31 PM

content hidden as private in Bugzilla

saper added a comment.Dec 7 2013, 10:32 PM

... 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.

Restricted Application removed a subscriber: Liuxinyu970226. · View Herald TranscriptSep 11 2016, 9:13 AM
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Migrating from the old tracking task to a tag for PostgreSQL-related tasks.