Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.This issue is part of #2157455: [Meta] Make Drupal 8 work with PostgreSQL or remove support from core before release.
Problem/Motivation
We have failing tests with PostgreSQL as Database backend in:
- DRUPAL\LOCALE\TESTS\LOCALEUPDATECRONTEST
- DRUPAL\SYSTEM\TESTS\SYSTEM\CRONQUEUETEST
The root cause is the claimItem() method that does not ensure to return the queue items in the correct order. This is because claimItem() orders the items by creation date. The creation date is a timestamp (Unix time), but that is problematic because many items get the same timestamp, e.g. created within the same second.
Proposed resolution
Change order by to additionally include the item_id to ensure correct ordering for queue items by ASC and to make PostgreSQL pass and to prevent possible random fails in MySQL.
User interface changes
None
API changes
None
Beta phase evaluation
| Issue category | Bug because of broken tests |
|---|---|
| Issue priority | Major because of broken test environment |
| Disruption | None disruptive for core/contributed and custom modules/themes because it is a bugfix only |
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 2356983-claimItem-order-item-id.patch | 940 bytes | Dave Reid |
| #10 | locale_group_tests-2.txt | 2.24 KB | jaredsmith |
| #9 | claim-item-2.patch | 1 KB | bzrudi71 |
| #5 | claim-item.patch | 1015 bytes | bzrudi71 |
| #1 | locale_group_tests.txt | 269.37 KB | bzrudi71 |
Comments
Comment #1
bzrudi71 CreditAttribution: bzrudi71 commentedJust two fails in LocaleUpdateCronTest...
Comment #2
bzrudi71 CreditAttribution: bzrudi71 commentedComment #3
jaredsmith CreditAttribution: jaredsmith commentedRe-ran the tests using the drupalci test infrastructure, with PHP 5.5 and PostgreSQL 9.1. Still seeing the same two failed tests.
Test run started:
Friday, February 13, 2015 - 16:16
Test summary
------------
Drupal\locale\Tests\LocaleConfigManagerTest 5 passes
Drupal\locale\Tests\LocaleConfigTranslationImportTest 30 passes
Drupal\locale\Tests\LocaleConfigSubscriberTest 87 passes
Drupal\locale\Tests\LocaleConfigTranslationTest 93 passes
Drupal\locale\Tests\LocaleJavascriptTranslationTest 46 passes
Drupal\locale\Tests\LocaleExportTest 59 passes
Drupal\locale\Tests\LocaleLibraryAlterTest 2 passes
Drupal\locale\Tests\LocaleContentTest 128 passes
Drupal\locale\Tests\LocaleLocaleLookupTest 22 passes
Drupal\locale\Tests\LocalePathTest 47 passes
Drupal\locale\Tests\LocaleStringTest 28 passes
Drupal\locale\Tests\LocaleTranslatedSchemaDefinitionTest 18 passes
Drupal\locale\Tests\LocaleTranslationProjectsTest 4 passes
Drupal\locale\Tests\LocaleTranslateStringTourTest 22 passes
Drupal\locale\Tests\LocalePluralFormatTest 184 passes
Drupal\locale\Tests\LocaleUpdateCronTest 50 passes 2 fails
Drupal\locale\Tests\LocaleUpdateInterfaceTest 58 passes
Drupal\locale\Tests\LocaleImportFunctionalTest 321 passes
Drupal\locale\Tests\LocaleTranslationUiTest 338 passes
Drupal\locale\Tests\LocaleUpdateTest 427 passes
Test run duration: 8 min 28 sec
Comment #4
jaredsmith CreditAttribution: jaredsmith commentedThe two failing tests in LocaleUpdateCronTest are because the timestamps aren't being properly updated in the database during this test. Still trying to figure out why, but it seems to be the call to the
db_mergefunction in thelocale_translation_update_file_historyfunction that's not working correctly.I've got limited connectivity right now (as I'm on an airplane, and will be traveling all day), but maybe someone can take a look at db_merge and ensure it's working as it should in PostgreSQL. If not, I'll continue to look at it as soon as I'm able.
Comment #5
bzrudi71 CreditAttribution: bzrudi71 commented@jaredsmith: Based on your information I tried to trace this down. In addition to this, I found out that we have a fail in the
itself, so I looked at this first.
With the help of some debug I hopefully found the (tricky) root cause in
Drupal\Core\Queue\QueueInterface::claimItem(). The claimItem() method returns the items ordered by creation time. And this is not save, as all items in test are created within the same second and get the same unix time timestamp. I changed the order to make use the item_id as argument and this seems to work now.Let's see if the attached patch works for MySQL also, and if so we need to test this on PG bot too.
Thanks!
Comment #6
bzrudi71 CreditAttribution: bzrudi71 commentedWe have pass for MySQL, so changed issue summary to explain the problem in detail.
Comment #7
bzrudi71 CreditAttribution: bzrudi71 commentedNice, patch makes tests pass on PG bot now, so please review.
Comment #8
tstoecklerI think in general (and conceptually) ordering by created makes sense (thinking of a FIFO queue) so maybe we can order by created first and then by item_id?
Comment #9
bzrudi71 CreditAttribution: bzrudi71 commentedSure we can. Changed issue summary to reflect the now additionally order by item_id.
Comment #10
jaredsmith CreditAttribution: jaredsmith commentedDCI_PHPVERSION=5.5
DCI_DRUPALBRANCH=8.0.x
DCI_TESTGROUPS=locale
DCI_UPDATEREPO=TRUE
DCI_DBTYPE=pgsql
DCI_DBVER=9.1
DCI_PATCH=https://www.drupal.org/files/issues/claim-item-2.patch
Reviewed and tested both manually and with the drupalci infrastructure support for pgsql. With the patch from #2356983-9: Fix SystemQueue::claimItem() returns items in the wrong order (resolves locale test failures on PostgreSQL), all tests pass on PostgreSQL using the above parameters. Setting as RTBC, as it's a simple one-line patch and obviously does the right thing with queues.
Comment #11
webchickCommitted and pushed to 8.0.x. Thanks!
Comment #14
Dave ReidThis should have been marked for backport for Drupal 7, I've encountered queue items being processed out of order when they all contain the same timestamp as well.
Comment #15
jaredsmith CreditAttribution: jaredsmith commentedPatch in comment 14 works as expected. I think this is ready to be committed to the 7.x branch now as well.
Comment #16
Dave ReidRe-titling to be more clear this is a bug in the queue, not necessarily PostgreSQL or locale tests.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!