Problem/Motivation

We could leverage more on the event system to manage database schema operations.

For example:

1. #3410312-12: Flood database backend ::isAllowed() should call ::ensureTableExists()

@mondrake

[...] I find on-the-fly table creation a rather bad approach. If we had the table created at installation, we wouldn’t have to care about this. On-the-fly table creation breaks transactions for DBs that do not support transactional DDL - not just the transaction where ensureTable is called, also any concurrent transaction from other sessions. [...]

@catch

[...] we've got a few more options now - for example services that need to create tables could subscribe to a module installed event and create the storage then [...]

2. #3410312-14: Flood database backend ::isAllowed() should call ::ensureTableExists()

@mondrake

[...] events would help in #3397622: Adding GIN and GIST indexes to PostgreSQL databases, to allow database drivers to decorate db objects specifications with db specific features [...]

Steps to reproduce

Proposed resolution

Remaining tasks

Discuss

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#7 3410480-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3410480

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Assigned: Unassigned » mondrake

I am working on a PoC for the second use case from the IS.

mondrake’s picture

Title: Use events in Database Schema operations » [META] Use events in Database Schema operations

Here's the idea of the PoC.

We have the event service available since the very first DDL statement executed during an installation, so we can build around it. (Exception, currently - Drush does not register services defined by the database driver's module, so an event subscriber defined into it won't execute - see https://github.com/drush-ops/drush/issues/5846).

Call the following 'hook_schema_alter strikes back' if you're a fan of the Force.

1. We replace the current way of defining a table's structure via a nested array, with a structure of value objects defined in the Drupal\Core\Database\SchemaDefinition namespace. This structure is immutable and database abstract. For example, the db-specific field type array key 'mysql_type', or the 'mysql_engine' will no longer be defined here.

A table currently represented like

    $schema = [
      'description' => 'The base table for configuration data.',
      'fields' => [
        'collection' => [
          'description' => 'Primary Key: Config object collection.',
          'type' => 'varchar_ascii',
          'length' => 255,
          'not null' => TRUE,
          'default' => '',
        ],
        'name' => [
          'description' => 'Primary Key: Config object name.',
          'type' => 'varchar_ascii',
          'length' => 255,
          'not null' => TRUE,
          'default' => '',
        ],
        'data' => [
          'description' => 'A serialized configuration object data.',
          'type' => 'blob',
          'not null' => FALSE,
          'size' => 'big',
        ],
      ],
      'primary key' => ['collection', 'name'],
    ];

Will then become sth like

    new Table(
      name: 'config',
      description: 'The base table for configuration data.',
      columns: [
        new Column(
          name: 'collection',
          description: 'Primary Key: Config object collection.',
          type: 'varchar_ascii',
          length: 255,
          notNull: TRUE,
          default: '',
        ),
        new Column(
          name: 'name',
          description: 'Primary Key: Config object name.',
          type: 'varchar_ascii',
          length: 255,
          notNull: TRUE,
          default: '',
        ),
        new Column(
          name: 'data',
          description: 'A serialized configuration object data.',
          type: 'blob',
          notNull: FALSE,
          size: 'big',
        ),
      ],
      primaryKey: new PrimaryKey(['collection', 'name']),
    );

2. This strawman structure is then used to build a concrete specification of Drupal\Core\Database\Schema namespace objects or an override of any of them in the database driver module itself - via an event subscriber. Using the event system allows ANY subscriber to alter the CONCRETE specification, keeping the abstract definition untouched. This will allow defining any db-specifc property (for example, the type of index for #3397622: Adding GIN and GIST indexes to PostgreSQL databases) in the overriding classes, and we will be able to have full static analysis of the code with defined properties.

3. The CONCRETE specification is then used by the drivers' Schema class to actually perform the DDL statements necessary. Note how, most likely, a large part of what is in those classes today will be moved to the subscriber instead.

The MR is a PoC, it runs OK a number of tests locally, but fails PHPCS and PHPStan. I am not going to fix that though, I'd rather appreciate feedback on the concept.

Turning this issue into a meta. My next step will be to start working on point 1 above, which is all by itself a large endeavour. Will open a separate issue for that.

mondrake’s picture

Status: Active » Needs review
Issue tags: +Needs subsystem maintainer review, +Needs framework manager review
mondrake’s picture

Assigned: mondrake » Unassigned
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: +no-needs-review-bot

nod_ made their first commit to this issue’s fork.

daffie’s picture

@mondrake: Could you explain to me how with this change, it would help with #3397622: Adding GIN and GIST indexes to PostgreSQL databases. Also with DB's that do not support transactional DDL. Why does this change make it better for those DB's? I missing why with this change it will be better.

catch’s picture

Category: Feature request » Plan
Issue tags: +Needs issue summary update

Moving this to a plan.

We discussed the general problem (on-the-fly table creation and transactions) at Drupal dev days but I don't think that discussion made it back into an issue, this is probably the right one.

Generally, I still think it's a good idea to make database-implementations-of-services (cache, lock, flood etc.) responsible for creating their database schema, so that if you have a redis backend installed, you don't have 10 empty cache tables in your database for no reason.

However, we could use an interface + tagged services to allow those services to create their schema during module install at an explicit point, so that the fallback is only necessary if you change services.yml to switch an implementation (e.g. from redis to sql) on runtime. And we could maybe do the same tagged services check on a cache clear all so that sites can trigger table creation without having to wait for an entity save or something to do it for them.

This way we'd still have on-demand table creation but we'd be able to... demand it.

mondrake’s picture

Status: Needs review » Active

This issue being now a plan, IMHO the MR should be moved to a separate issue (if needed).

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.