Problem

  1. Connection::query() is designed to contain SELECT statements only.
  2. rowCount() on SELECT is illegal.
  3. We have enforced this in a recent patch which makes $database->query->rowCount() blow up.
  4. The SQLite and Postgesql drivers runs query with UPDATE. This is not supported but because it is inside the database driver it gets a pass.

As a corollary, both drivers blows up and this is by design.

Proposed resolution

Change the illegal query that drivers runs to better mimic db_update by setting the right return option. This actually shortens the code.

CommentFileSizeAuthor
#23 interdiff.rowcount-exception-message.txt1.03 KBsun
#16 2167507_16.patch2.68 KBandypost
#16 interdiff.txt615 bytesandypost
#15 2167507_15.patch2.66 KBandypost
#15 interdiff.txt596 bytesandypost
#13 2167507_13.patch2.08 KBandypost
#13 interdiff.txt769 bytesandypost
#10 2167507_10.patch1.33 KBchx
#9 2167507_9.patch1.17 KBchx
#8 2167507_8_Drupal8_does_not_install_on_SQLite_again.patch699 bytesdcrocks
installhang.jpg39.46 KBdcrocks

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Priority: Normal » Critical

Confirmed.

git bisect should be able to help nail down the culprit.

aspilicious’s picture

aspilicious commented

did you remove the files directory when reinstalling??

dcrocks’s picture

dcrocks commented

I could not use 'git bisect' because the failure is in the middle of install. So I did a manual 'bisect' based on the calendar and think I have found the suspect. I did a clean clone of current 8.x and reverted the patch for issue #2146733: Select queries should not use rowCount() to calculate number of rows and install was successful. However I now see a lot of errors

Notice: Undefined index: _controller in Drupal\Core\Routing\Enhancer\AjaxEnhancer->enhance() (line 40 of /Users/rocks/Sites/drupal8/core/lib/Drupal/Core/Routing/Enhancer/AjaxEnhancer.php).

I will take a look at that patch for now.

andypost’s picture

andypost
he/him
Primary language Russian
commented

I don't think that rowCount() caused this because it useless for sqlite, suppose other issue caused this failure

dcrocks’s picture

dcrocks commented

However, D8 doesn't install on SQLite with the patch in, but does with the patch reverted. I'm not arguing to revert this patch, only to try to determine the error causing the install failure.

damien tournoud’s picture

damien tournoud commented

The StatementPrefetch has not been updated for this change. Not sure why it causes the issue.

dcrocks’s picture

dcrocks commented
Related issues: +#2146733: Select queries should not use rowCount() to calculate number of rows
dcrocks’s picture

dcrocks commented
Status: Active » Needs review
StatusFileSize
new 2167507_8_Drupal8_does_not_install_on_SQLite_again.patch699 bytes

I have a small patch that seems to fix the problem. Please, someone more knowledgeable about databases review this. And someone test it. My tests were of the trivial kind.

chx’s picture

chx commented
StatusFileSize
new 2167507_9.patch1.17 KB

Thanks for the bug report and the patch! It was revealing. But I believe this to be a more correct patch. I also added some comments.

chx’s picture

chx commented
StatusFileSize
new 2167507_10.patch1.33 KB

Kindly forget that :) I meant this.

chx’s picture

chx commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 10: 2167507_10.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Drupal8 does not install on SQLite - again » Drupal8 does not install on SQLite & Postgresql
Component: install system » database system
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt769 bytes
new 2167507_13.patch2.08 KB

pgsql needs the same fix after #2146733-22: Select queries should not use rowCount() to calculate number of rows

dcrocks’s picture

dcrocks commented

Patch installed cleanly and install of clone of D8 on SQLite was successful. Don't have PGsql to test.

Thanx @chx and @andypost for doing this right.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt596 bytes
new 2167507_15.patch2.66 KB

The proper fix for pgsql, tested with psql (9.3.2)

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt615 bytes
new 2167507_16.patch2.68 KB

Suppose we could close duplicate issue #2001350: [meta] Drupal cannot be installed on PostgreSQL

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

I tested with the patch and without the patch on PostgreSQL 9.2.4.

  • Without patch: install immediately white screens without any PHP error mesage OR Postgresql error message (other than cache tables).
  • With patch: install proceeds until it hits the issue below.

No, #2001350: [meta] Drupal cannot be installed on PostgreSQL should not be closed because PostgreSQL wasn't successfully installed. The install stopped at the same point from October, 2013, comment #74 re: comment_entity_statistics.

I think this issue should be renamed to be more specific to the actual issue it is trying to fix instead of attempting to fix "does not install", which is a meta issue now. So I think whatever this issue is trying to solve it may have solved it, but it did not fix the entire problem.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Title: Drupal8 does not install on SQLite & Postgresql » Fix rowCount query usage in pgsql and sqlite drivers

Changed title to be more specific.

dcrocks’s picture

dcrocks commented

Then can we focus on this, get it reviewed, and committed?

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

I am not sure why this wasn't RTBC'd by the testers above but apparently both databases are fixed so I will do the honors despite I wrote half of it.

chx’s picture

chx commented

Ps. I checked and the suggested change to allowRowCount in the postgresql driver query() now makes it match the generic query() .

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new interdiff.rowcount-exception-message.txt1.03 KB

Can we also throw a proper exception message, please? Often times, we're catching database exceptions and just $e->getMessage() or to re-throw them. Without a message, you have no clue what happened.

Potential fix attached as interdiff.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

Would including the query potentially expose sensitive information? I don't think so because it's probably going to be for php developers only who are trying to debug.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I don't want to hold this fix off any longer, because the broken SQLite is quite annoying for everyone working on the installer.

Hence moved into a separate issue: #2171359: Output proper exception message for database RowCountExceptions

Crell’s picture

Crell commented

Per request from chx, /me nods approvingly.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Awesome! Great work on this, folks. Looking forward to installing in SQLite again. :)

Committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.