READ COMMITTED requirement check doesn't account for database views

Created on 25 May 2023, over 2 years ago

Problem/Motivation

REPEATABLE-READ
The recommended level for Drupal is "READ COMMITTED". For this to work correctly, all tables must have a primary key. The following table(s) do not have a primary key: {insert long list of native mysql views}. See the setting MySQL transaction isolation level page for more information.
READ-COMMITTED
For this to work correctly, all tables must have a primary key. The following table(s) do not have a primary key: {insert long list of native mysql views}. See the setting MySQL transaction isolation level page for more information.

Anyone that's worked with views will immediately know that this is because in the schema API and some other places, a view shows up largely like a table. I assume this is a pretty useful feature but it can also be frustrating if you don't take it into account in situations like this.

Steps to reproduce

1. Install Drupal
2. run this on your database. CREATE VIEW test AS SELECT * FROM users;
3. Visit the status page and see one of the above warning or errors.

Proposed resolution

This might be tricky. Adjusting findTables to not include views might be too big a change? That said, I don't see an interface for listing or interacting with what type of "table" you're interacting with so only including real tables might make sense.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Active

Version

10.1

Component
MySQL driver 

Last updated 28 days ago

Created by

🇺🇸United States neclimdul Houston, TX

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @neclimdul
  • 🇳🇱Netherlands daffie

    Bumping this to major. This is very confusing for site owners. Would be great if we could fix this before Drupal 10.1 is released. Maybe this should even be a "critical" one.

  • 🇷🇺Russia Chi

    The issue only applies to MySQL/MariaDB views. Right?

  • 🇳🇱Netherlands daffie

    @chi you are right.

  • 🇬🇧United Kingdom catch

    I think it would be reasonable to restrict ::findTables() to only finding actual database tables - we could do that with a change record probably.

    I tried to think of bc-friendly options (adding a second argument for $include_views = TRUE? adding a settings override?) but couldn't come up with a use case.

  • 🇺🇸United States neclimdul Houston, TX

    I guess this just needs someone to write a patch?

  • Status changed to Needs work about 2 years ago
  • 🇺🇸United States jrb Raleigh-Durham Area, NC, USA

    Here's a patch to get things started. This will:

    1. Add an optional parameter to findTables() to exclude SQL views from the query.
    2. Use that parameter in the mysql_requirements() call in mysql.install.
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update about 2 years ago
    Custom Commands Failed
  • 🇺🇸United States anoopjohn Washington D. C.

    The patch works correctly on a 10.1.6 drupal site and stops showing warnings against views in the database.

  • 🇺🇸United States micahw156

    Confirming this patch worked for me on 10.1.6 as well.

  • 🇦🇺Australia nterbogt

    Confirm this works for me too.

  • 🇦🇺Australia apiddington

    Confirming that this worked OK with 10.2.

  • 🇦🇺Australia apiddington

    Patch at #8 no longer works with v11.2
    It seems that core/modules/mysql/mysql.install has been replaced by core/modules/mysql/src/Hook/MysqlRequirements.php

    This patch seems to work OK with v11.2: 3362726-9.exclude-views-from-table-list-11.2.patch

  • 🇺🇸United States nicxvan

    Can you convert this to an MR?

    Tests don't run against patches.

  • First commit to issue fork.
  • Merge request !12436Draft: Resolve #3362726 "Read committed requirement" → (Open) created by oily
  • 🇬🇧United Kingdom oily Greater London

    Added new test function.

  • Pipeline finished with Failed
    3 months ago
    Total: 222s
    #528128
  • 🇺🇸United States nicxvan

    Did you accidentally remove the actual change? I only see the test stub.

  • 🇬🇧United Kingdom oily Greater London

    I havent applied any of the patches yet. It needs to be done.

  • 🇬🇧United Kingdom oily Greater London

    findTables() has a new boolean parameter. The test would be setting the param to true in order to exclude views and allow only tables.

    The test will break against the current codebase because it will be setting a non-existent parameter. The code within the function is not going to be tested because the test wont reach it?

    Any ideas how such issues are correctly addressed in a kernel test?

  • Pipeline finished with Failed
    3 months ago
    Total: 159s
    #528201
  • 🇬🇧United Kingdom oily Greater London

    Okay, I see that the errors in the IS are triggered because views do not need to contain a primary key. There is existing code in other test functions involving the presence/ non-presence of primary keys. Possibly test coverage should be by extending such functions. Or perhaps can incorporate the primary key test code into the new test function..

    If test table (view) contains no primary key, check if findTables() 2nd parameter is set to TRUE. If it is, check if the table is a view. Assertion passes if it is a view with no primary key..

  • 🇬🇧United Kingdom oily Greater London

    The core Schema.php has a method createTable() but has no equivalent method to create views. The tests rely on createTable(). Not sure it is worthwhile or feasible to support views in the piecemeal fashion of this issue.

    If we are going to take cognisance of views should be not build it into the database api including the core Schema.php. As it stands we will need, i think, to use raw sql queries in the tests ie CREATE VIEW statements as mentioned in the steps to reproduce.

  • 🇬🇧United Kingdom oily Greater London

    If we could add minimal support for DB views into core say by adding createView() method, possibly related CRUD methods too. Then do minor refactor of existing test functions eg the one that tests tables all have primary keys. So the test function would do a 'it has not got a primary key. Is it a view? Yes? In which case, no problem.'

  • 🇺🇸United States nicxvan

    I think for this issue we would manually run asql command to create a db view on the test db.

    Creating an api for creating and managing db views is it if scope here and not necessary.

    I don't know how we go about doing my recommendation here, just giving thoughts on approach and scope.

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    I don't think we should change the default behaviour of findTables()... if we do we'll break any tests for contrib or custom code which are creating a view at the moment as we rely on findTables() to clear up test databases. Maybe we can introduce a flag that defaults to the current behaviour but allows you to optionally list only real tables or only views.

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    That said I guess

        if ($original_prefix != $test_prefix) {
          $tables = Database::getConnection()->schema()->findTables('%');
          foreach ($tables as $table) {
            if (Database::getConnection()->schema()->dropTable($table)) {
              unset($tables[$table]);
            }
          }
        }
    

    is flawed because dropTable is not going to work... and looking at our dropTable() implementation

      /**
       * {@inheritdoc}
       */
      public function dropTable($table) {
        if (!$this->tableExists($table)) {
          return FALSE;
        }
    
        $this->executeDdlStatement('DROP TABLE {' . $table . '}');
        return TRUE;
      }
    

    Well the tableExists call is going to return false... so it's not going to even error.

    Also I think both SQLite and Postgres are unaffected because their implementations of findTables() is different.

    So... tldr; perhaps fixing findTables() to only return tables for MySQL is actually the way to go here!

  • Merge request !12442Prove MySQL is the outlier → (Closed) created by alexpott
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    Just added an MR that shows that the MySQL driver is on it's own here by returning views from findTables()... so I think we're okay just to remove the capability from MySQL. Discussions around a database views API can happen elsewhere.

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    My MR now also includes a fix that's limited to MySQL (and MySQLi) where the problem occurs.

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    alexpott changed the visibility of the branch 3362726-read-committed-requirement to hidden.

  • Pipeline finished with Canceled
    3 months ago
    Total: 875s
    #528430
  • Pipeline finished with Failed
    3 months ago
    Total: 673s
    #528432
  • Pipeline finished with Success
    3 months ago
    Total: 4078s
    #528465
  • 🇬🇧United Kingdom oily Greater London

    @alexpott

    I added code comment to: https://git.drupalcode.org/project/drupal/-/merge_requests/12442/diffs?c...

    Is that commit necessary for filtering views amongst tables?

  • 🇬🇧United Kingdom oily Greater London

    +1 #27

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    #27 is wrong. See #28 - if your module creates a view it'll error on trying to clean up a test in Postgres - not sure what it'll do for MySQL but the drop table is certainly not going to clean up the table as expected. All of our code is assuming findTable() only returns tables - which is excatly how it works for Postgres and SQLite... it just doesn't work this way for MySQL. I would argue that findTables() shoudl only find tables because of this and that the current MySQL behaviour is the outlier. The tests and changes added here prove this.

    @oily the MySQL 5 thing is altered so that getComment() has a consistent return value for both MySQL and Postgres. FWIW the MySQL implementation has no business changing the FALSE to an empty string because of the preg_replace.

  • Pipeline finished with Success
    3 months ago
    Total: 6873s
    #528488
  • 🇬🇧United Kingdom oily Greater London

    @alexpott The commit I queried.. I understand the purpose now. Line 688 reveals the relevance to this issue. If i had scrolled up a bit I would have seen it.

  • Pipeline finished with Success
    3 months ago
    #528540
  • 🇬🇧United Kingdom oily Greater London

    Test coverage in place. Test-only test fails as it should.

    Can move to RTBTC?

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    @oily yes you can.

  • 🇬🇧United Kingdom oily Greater London
  • Pipeline finished with Failed
    3 months ago
    Total: 168s
    #529150
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    I created the CR as per #6

  • Pipeline finished with Success
    3 months ago
    Total: 2861s
    #529176
  • 🇬🇧United Kingdom oily Greater London

    Reviewed CR. 'materialized' views is interesting. Just looked it up. Need to research further. Asking myself 'how to create material view as opposed to a normal view..' Could CR benefit from example Mysql query to create a material view? The example is the IS is: "CREATE VIEW test AS SELECT * FROM users;" That does create a 'material' view?

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    @oily it's just what they are called - see https://en.wikipedia.org/wiki/Materialized_view - also I felt it was important to distinguish from Drupal's use of the views as in the views module.

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    Ugh... nah I'm wrong. DBs implement both views and materialized views... old wires crossed in my head.

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    Gone for "database views" as the issue title here has... @oily thanks for questioning this!

  • 🇬🇧United Kingdom oily Greater London

    @alexpott Thanks for considering my suggestion. The CR looks good!

    • catch committed d5002caa on 11.x
      Issue #3362726 by alexpott, oily, jrb, neclimdul: READ COMMITTED...
    • catch committed 4a1d887a on 11.2.x
      Issue #3362726 by alexpott, oily, jrb, neclimdul: READ COMMITTED...
    • catch committed 0881e207 on 11.2.x
      Revert "Issue #3362726 by alexpott, oily, jrb, neclimdul: READ COMMITTED...
  • 🇬🇧United Kingdom catch

    Committed/pushed to 11.x, thanks!

    I backported this to 11.2.x, then immediately changed my mind and reverted it again after re-reading the CR. This is an annoying bug (although for site admins rather than site visitors) but the important thing is to fix it in a branch of core, which was non-trivial, so not worth risking the theoretical disruption of an 11.2 or 10.x backport.

  • Status changed to Fixed 28 days ago
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024