Gerrit change #41020, tested on MySQL, PostgreSQL (for the most part) and SQLite for:

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

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

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.

content hidden as private in Bugzilla

content hidden as private in Bugzilla

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

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