In T225084#5236564, @alaa_wmde wrote:

Current error I get when we try to write to both stores:

Warning:  [data-update-failed]: A data update callback triggered an exception (Wikibase\Lib\Store\Sql\Terms\DatabaseTermIdsCleaner::cleanTermIds: Transaction round 'Wikibase\Repo\Content\DataUpdateAdapter::doUpdate' already started

Called from Wikibase\Repo\Content\DataUpdateAdapter::doUpdate in /srv/mediawiki/html/extensions/Wikibase/repo/includes/Content/DataUpdateAdapter.php at line 65] in /srv/mediawiki/html/includes/debug/MWDebug.php  on line 309

will investigate locally now

So this works fine locally .. maybe has something to do with test environment setup?

In T225084#5236601, @alaa_wmde wrote:

In T225084#5236564, @alaa_wmde wrote:

Current error I get when we try to write to both stores:

Warning:  [data-update-failed]: A data update callback triggered an exception (Wikibase\Lib\Store\Sql\Terms\DatabaseTermIdsCleaner::cleanTermIds: Transaction round 'Wikibase\Repo\Content\DataUpdateAdapter::doUpdate' already started

Called from Wikibase\Repo\Content\DataUpdateAdapter::doUpdate in /srv/mediawiki/html/extensions/Wikibase/repo/includes/Content/DataUpdateAdapter.php at line 65] in /srv/mediawiki/html/includes/debug/MWDebug.php  on line 309

will investigate locally now

So this works fine locally .. maybe has something to do with test environment setup?

Ah it might as well be related to https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/514463.
I will test again with that patch included on test node

In T225084#5236606, @alaa_wmde wrote:

In T225084#5236601, @alaa_wmde wrote:

In T225084#5236564, @alaa_wmde wrote:

Current error I get when we try to write to both stores:

Warning:  [data-update-failed]: A data update callback triggered an exception (Wikibase\Lib\Store\Sql\Terms\DatabaseTermIdsCleaner::cleanTermIds: Transaction round 'Wikibase\Repo\Content\DataUpdateAdapter::doUpdate' already started

Called from Wikibase\Repo\Content\DataUpdateAdapter::doUpdate in /srv/mediawiki/html/extensions/Wikibase/repo/includes/Content/DataUpdateAdapter.php at line 65] in /srv/mediawiki/html/includes/debug/MWDebug.php  on line 309

will investigate locally now

So this works fine locally .. maybe has something to do with test environment setup?

Ah it might as well be related to https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/514463.
I will test again with that patch included on test node

Testing with that patch didn't change the situation .. still same error when trying to clean up terms of an updated property

Damn, I also thought it would be fixed with that change…

The tests are working:

lucaswerkmeister-wmde@wikidata-new-wbterm:/srv/mediawiki/html$ php tests/phpunit/phpunit.php extensions/Wikibase/lib/tests/phpunit/Store/Sql/Terms/
Using PHP 7.0.33-0+deb9u3
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

................................................................. 65 / 70 ( 92%)
.....                                                             70 / 70 (100%)

Time: 1.69 seconds, Memory: 32.00MB

OK (70 tests, 177 assertions)

I think I’ve figured it out. Stacktrace from log/debug-termwiki.log:

[Wikibase] Wikibase\Lib\Store\Sql\Terms\DatabasePropertyTermStore::storeTerms: DBError while storing terms for P1: [Exception Wikimedia\Rdbms\DBTransactionError( /srv/mediaw
iki/html/includes/libs/rdbms/lbfactory/LBFactory.php:247) Wikibase\Lib\Store\Sql\Terms\DatabasePropertyTermStore::storeTerms: transaction round 'Wikibase\Repo\Content\DataUp
dateAdapter::doUpdate' already started.]
#0 /srv/mediawiki/html/extensions/Wikibase/lib/includes/Store/Sql/Terms/DatabasePropertyTermStore.php(108): Wikimedia\Rdbms\LBFactory->beginMasterChanges(string)
#1 /srv/mediawiki/html/extensions/Wikibase/lib/includes/Store/MultiPropertyTermStore.php(47): Wikibase\Lib\Store\Sql\Terms\DatabasePropertyTermStore->storeTerms(Wikibase\Dat
aModel\Entity\PropertyId, Wikibase\DataModel\Term\Fingerprint)
#2 /srv/mediawiki/html/extensions/Wikibase/lib/includes/Store/DelegatingEntityTermStoreWriter.php(51): Wikibase\MultiPropertyTermStore->storeTerms(Wikibase\DataModel\Entity\
PropertyId, Wikibase\DataModel\Term\Fingerprint)
#3 /srv/mediawiki/html/extensions/Wikibase/lib/includes/Store/DelegatingEntityTermStoreWriter.php(34): Wikibase\Lib\Store\DelegatingEntityTermStoreWriter->storeProperty(Wiki
base\DataModel\Entity\Property)
#4 [internal function]: Wikibase\Lib\Store\DelegatingEntityTermStoreWriter->saveTermsOfEntity(Wikibase\DataModel\Entity\Property)
#5 /srv/mediawiki/html/extensions/Wikibase/repo/includes/Content/DataUpdateAdapter.php(62): call_user_func_array(array, array)
#6 /srv/mediawiki/html/includes/deferred/DeferredUpdates.php(309): Wikibase\Repo\Content\DataUpdateAdapter->doUpdate()
#7 /srv/mediawiki/html/includes/Storage/DerivedPageDataUpdater.php(1613): DeferredUpdates::attemptUpdate(Wikibase\Repo\Content\DataUpdateAdapter, Wikimedia\Rdbms\LBFactorySi
mple)
#8 /srv/mediawiki/html/includes/Storage/DerivedPageDataUpdater.php(1432): MediaWiki\Storage\DerivedPageDataUpdater->doSecondaryDataUpdates(array)
#9 /srv/mediawiki/html/includes/deferred/MWCallableUpdate.php(38): MediaWiki\Storage\DerivedPageDataUpdater->MediaWiki\Storage\{closure}()
#10 /srv/mediawiki/html/includes/deferred/DeferredUpdates.php(304): MWCallableUpdate->doUpdate()
#11 /srv/mediawiki/html/includes/deferred/DeferredUpdates.php(265): DeferredUpdates::attemptUpdate(MWCallableUpdate, Wikimedia\Rdbms\LBFactorySimple)
#12 /srv/mediawiki/html/includes/deferred/DeferredUpdates.php(217): DeferredUpdates::handleUpdate(MWCallableUpdate, Wikimedia\Rdbms\LBFactorySimple, string, integer)
#13 /srv/mediawiki/html/includes/deferred/DeferredUpdates.php(144): DeferredUpdates::handleUpdateQueue(array, string, integer)
#14 /srv/mediawiki/html/includes/MediaWiki.php(907): DeferredUpdates::doUpdates(string)
#15 /srv/mediawiki/html/includes/MediaWiki.php(731): MediaWiki->restInPeace(string, boolean)
#16 /srv/mediawiki/html/includes/MediaWiki.php(754): MediaWiki->{closure}()
#17 /srv/mediawiki/html/api.php(124): MediaWiki->doPostOutputShutdown(string)
#18 {main}

And the reason, at its core, is very simple. Wikibase already defers term updates. And unless an update explicitly declares itself to be a TransactionRoundAwareUpdate (i. e. it manages its own transaction(s)), MediaWiki core’s DeferredUpdates::attemptUpdate() wraps the whole update in a transaction named after the update. In Wikibase’s case, that name is the somewhat unhelpful DataUpdateAdapter::doUpdate, which doesn’t hint at what’s really inside that update adapter – but the update that fails is generated in EntityHandler::getSaveTermUpdates(): a deferred update for calling TermStoreWriter::saveTermsOfEntity.

So I think this means two things for us:

1. Don’t bother deferring the cleanup (T221704) – the whole terms update is already deferred, there’s no need to defer the last part of it even more.
2. In Database{Property,Item}TermStore, either remove the whole transaction code, or guard it with some kind of “if we’re not already in a transaction” condition (ILBFactory::hasTransactionRound(), I believe).

Also, I don’t know why we couldn’t reproduce this error locally before, but right now I can reproduce it (on If976a3d99f, PS1). So that’s something, I guess.

I am for the two suggestions you laid out above. Let's conditionally handle transactions if possible, or remove it and keep it all up to the user of these stores to handle transactions if we cannot do it conditionally in an easy way.

I just wonder whether restoration logic would still work if everything is actually wrapped in one transaction round.. but I think that can be tested (esp. after we have merged If976a3d99f, PS1 and resolved separately. I cannot think of a reason why that shouldn't work atm.

• Don’t bother deferring the cleanup (T221704) – the whole terms update is already deferred, there’s no need to defer the last part of it even more.

moved T221704 to Stalled, we can abandon it once we tested everything after the next point is done.

• In Database{Property,Item}TermStore, either remove the whole transaction code, or guard it with some kind of “if we’re not already in a transaction” condition (ILBFactory::hasTransactionRound(), I believe).

created T225348 to remove transaction control code in these classes for now.

Property writing seems to work now – I updated the install and then edited the English label of Property:P1 a few times, and after each edit the following query returned the correct text:

SELECT wbpt_property_id, wby_name, wbxl_language, wbx_text
FROM wbt_property_terms
JOIN wbt_term_in_lang ON wbpt_term_in_lang_id = wbtl_id
JOIN wbt_type ON wbtl_type_id = wby_id
JOIN wbt_text_in_lang ON wbtl_text_in_lang_id = wbxl_id
JOIN wbt_text ON wbxl_text_id = wbx_id
WHERE wbpt_property_id = 1
AND wbxl_language = 'en'
AND wby_name = 'label';

SELECT wbpt_property_id, wby_name, wbxl_language, wbx_text FROM wbt_property_terms JOIN wbt_term_in_lang ON wbpt_term_in_lang_id = wbtl_id JOIN wbt_type ON wbtl_type_id = wby_id JOIN wbt_text_in_lang ON wbtl_text_in_lang_id = wbxl_id JOIN wbt_text ON wbxl_text_id = wbx_id WHERE wbpt_property_id = 1 AND wbxl_language = 'en' AND wby_name = 'label';

And items like Item:Q1 seem to work just as well after I enabled item migration.

SELECT wbit_item_id, wby_name, wbxl_language, wbx_text FROM wbt_item_terms JOIN wbt_term_in_lang ON wbit_term_in_lang_id = wbtl_id JOIN wbt_type ON wbtl_type_id = wby_id JOIN wbt_text_in_lang ON wbtl_text_in_lang_id = wbxl_id JOIN wbt_text ON wbxl_text_id = wbx_id WHERE wbit_item_id = 1 AND wbxl_language = 'en' AND wby_name = 'label';

And cleanup also seems to work fairly well – wbx_text rows for the old versions of the labels I edited are gone.

I'm testing now including item terms migration with upper bound id, with the setting:

[ 15000 => MIGRATION_NEW, 50000 => MIGRATION_WRITE_BOTH, 100000 => MIGRATION_WRITE_NEW, 'max' => MIGRATION_OLD ]

In T225084#5252574, @alaa_wmde wrote:

I'm testing now including item terms migration with upper bound id, with the setting:

[ 15000 => MIGRATION_NEW, 50000 => MIGRATION_WRITE_BOTH, 100000 => MIGRATION_WRITE_NEW, 'max' => MIGRATION_OLD ]

Test results with that setting:

• Updating Q100, updated only new store as expected
• Updating Q15000, updated only new store as expected

• Updating Q26000, updated both stores as expected
• Updating Q50000, updated both stores as expected

• Updating Q70000, updated both stores as expected (we deliberately keep writing to both stores even in MIGRATION_WRITE_NEW stage)
• Updating Q100000, updated both stores as expected

• Updating Q200000, updated only old store as expected

Clean up seems also to work for the above testing.. I can't find the old texts in relevant stores anymore.

Great, thanks for testing!