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.
Currently there is one or more fail or exception in the search test group identified by the new docker based testbot that need to be fixed to make PostgreSQL finally passing all tests. Command in use:
sudo DCI_DBTYPE='pgsql' \
DCI_DBVER='9.1' \
DCI_PHPVERSION='5.5' \
DCI_CONCURRENCY='4' \
DCI_TESTGROUPS='search' \
DCI_DRUPALBRANCH='8.0.x' \
DCI_VERBOSE='true' \
DCI_UPDATEREPO='true' \
./containers/web/run.sh
Beta phase evaluation
Issue category | Bug because it breaks tests on PostgreSQL |
---|---|
Issue priority | Major |
Disruption | Non Disruptive for core/contributed and custom modules/themes because it fixes a bug only |
Comment | File | Size | Author |
---|---|---|---|
#20 | postgresql-search-group-fix-20.patch | 563 bytes | bzrudi71 |
Comments
Comment #1
bzrudi71 CreditAttribution: bzrudi71 commentedGROUP BY error and some others...
Comment #2
bzrudi71 CreditAttribution: bzrudi71 commentedThis should fix broken SearchBlockTests as a first start. Let's see what MySQL bot thinks...
Comment #3
bzrudi71 CreditAttribution: bzrudi71 commentedGreat, patch fixes broken SearchBlockTest and SearchPageTextTest. However while working on the remaining issues I noticed some strange random test fails because of database is locked errors. This happens namely in SearchNumberMatchingTest and SearchRankingTest as a handful of runs show.
I also run the search test group with MySQL and the same random fails show up, so it's not really PostgreSQL only. Anyway, I have no real idea how to work around that yet, so any help is welcome here ;-)
I attached the PostgreSQL and MySQL test results for review.
Comment #4
bzrudi71 CreditAttribution: bzrudi71 commentedGuess this could be caused by a still running cron job? I mean, search tests make often use of cron for indexing... needs investigation
Comment #6
bzrudi71 CreditAttribution: bzrudi71 commentedI found that the 'database is locked' issue happens even outside of search test group, so that will need a separate issue. Patch still applies and passes MySQL bot so I'm setting to RTBC as it' a just single line change and does fix PG fails.
Comment #7
bzrudi71 CreditAttribution: bzrudi71 commentedOpened #2420909: Random database is locked errors
Comment #8
bzrudi71 CreditAttribution: bzrudi71 commentedComment #9
webchickPlease don't mark your own patch "Reviewed & tested by the community"; the purpose of that status is to signify that peer review has been completed.
The patch itself actually touches search module, so reassigning to that component. Either pwolanin or jhodgdon would be good people to reach out to for review.
Comment #10
bzrudi71 CreditAttribution: bzrudi71 commentedOkay thanks @webchick! So let's wait for a search maintainer review then... Just to make sure, attaching the PostgreSQL test run log showing 100% passing tests with patch applied.
Comment #11
jhodgdonI am having trouble understanding this.
Could you please post a shorter log that just shows the actual errors that are occurring without the patch in PostgreSQL? The full test log is huge.
I am also not sure about the patch, or whether it will work in all cases. What you've done is:
But is.sid is already in the query, even if the count($this->conditions) evaluates to FALSE. So wouldn't the group by be needed even outside the if() statement? Lower down in the same function:
so it seems like you would need the groupBy no matter what?
It is quite possible that our test coverage doesn't include a case where there are no conditions coming into this function... but I'm uncomfortable endorsing this fix as making the Search module actually work well with PostgreSQL in all cases. We may need more tests added to cover that bit of code.
Comment #12
andypostTested that,
\Drupal\search\Tests\SearchBlockTest fails when only one character passed to search, there's actually no grouping happens
/search/node?keys=a
The second test is
Looks the same - one letter search
Here's debugger
Comment #13
jhodgdonHm, I'm not convinced that in that case that SearchQuery::conditions() is empty.
Can you just try moving the added group by line outside the if(), like maybe just where the COUNT is added to the query, and see if that breaks anything? I'm really still not comfortable with where this is added.
Comment #14
andypostThe strange thing for me is that query is run when only one char but latter the form set message that keyword should be 3 chars at least.
Maybe it's a separate bug, but it makes sense to fix that here
Comment #15
jhodgdonThat seems like a completely separate issue from "PostgreSQL tests fail". Let's get a separate issue for that.
Comment #16
bzrudi71 CreditAttribution: bzrudi71 commented@jhodgdon, here is the actual error:
Yes, adding the group by only within the if condition feels wrong, but if I remember right I already tried it outside but with no luck. I will try that again later and post the results.
Comment #17
bzrudi71 CreditAttribution: bzrudi71 commentedOkay, here we go. Adding the groupBy right after! the if condition seems to work. That means we should now be save, even in case that the if condition is false...
Let's see if we pass MySQL with patch attached.
Comment #18
jhodgdonOK, great! MySQL seems to be happy too.
One nitpick:
You need a . at the end of the comment.
Also not sure why you removed the blank line near the end of the function.
Comment #19
andypostAlso this comment needs link to follow-up for #12, filed #2425759: SearchQuery lacks preExecute(), making countQuery() run even when there are problems in prepare step
Comment #20
bzrudi71 CreditAttribution: bzrudi71 commentedNew patch based on comments from #18.
Comment #21
jhodgdonOK, looks good now. Thanks for the updated patch!
Comment #22
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3ad86b6 and pushed to 8.0.x. Thanks!
@bzrudi71 is that a public postgres testbot result anywhere and if you're running the full suite against head can you publish the results somewhere. It would be great to know when we've got all of these fixed.
Comment #24
bzrudi71 CreditAttribution: bzrudi71 commented@alexpott: I have just setup a public (very simple) PostgreSQL Bot on Wednesday that runs all tests once a day. BTW, thanks for commit.
Comment #25
andypostThe problem was caused by order of execution of query extenders see #2425759-6: SearchQuery lacks preExecute(), making countQuery() run even when there are problems in prepare step
@bzrudi71 please test the patch in #2425759-7: SearchQuery lacks preExecute(), making countQuery() run even when there are problems in prepare step
Comment #26
jhodgdonThat patch didn't work in MySQL. But you could look at the later patch, and remove the added PostgreSQL line from this patch, and see if it still works in PostgreSQL. My guess is that it won't, because PostgreSQL does tend to be very picky about Group By.