Database configuration and migration #42

JanJakes · 2025-04-15T14:20:24Z

This PR extracts existing SQLite database configuration to a WP_SQLite_Configurator and implements WP_SQLite_Information_Schema_Reconstructor with the ability to backfill missing table data in information schema.

How it works:

The configurator checks the SQLite driver version against a version stored in the database, and if the driver is ahead, it will execute the database (re)configuration (see WP_SQLite_Configurator::ensure_database_configured()), while acquiring an EXCLUSIVE lock to prevent race conditions (multiple requests triggering the same migration).
When the configurator runs, it ensures that any internal tables are created (global variables, information schema), and then it proceeds to the WP_SQLite_Information_Schema_Reconstructor.
The reconstructor then compares existing SQLite tables against tables recorded in the information schema, backfills those that are missing, and removes those that no longer exist.
To backfill a table when in WordPress, and it is a WordPress table, the definition is taken from wp_get_db_schema().
To backfill a non-WordPress table, the information schema data is derived from _mysql_data_types_cache, if it exists, otherwise directly from the SQLite schema using SQLite column affinity rules.

…igration

.github/workflows/verify-version.yml

load.php

tests/WP_SQLite_Information_Schema_Reconstructor_Tests.php

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

adamziel · 2025-04-17T23:07:40Z

Great start! And also a good idea to use a cascade of data sources for backfilling. There's some more way to go, though — I've commented on a bunch of potential issues

…onfiguration

wp-includes/parser/class-wp-parser-token.php

wp-includes/parser/class-wp-parser.php

wp-includes/parser/class-wp-parser-node.php

wp-includes/sqlite-ast/class-wp-sqlite-driver.php

adamziel · 2025-04-25T15:40:15Z

.github/workflows/verify-version.yml

+
+jobs:
+  verify-version:
+    name: Verify that plugin version matches SQLITE_DRIVER_VERSION constant


What do you think about this slightly longer name?

Suggested change

name: Verify that plugin version matches SQLITE_DRIVER_VERSION constant

name: Assert the WordPress plugin header declares the same version as the SQLITE_DRIVER_VERSION constant

adamziel · 2025-04-25T15:47:00Z

wp-includes/sqlite-ast/class-wp-sqlite-driver.php

-			$parser = new WP_MySQL_Parser( self::$mysql_grammar, $tokens );
-			$ast    = $parser->parse();
-
+			// TODO: Translate and execute all queries in the SQL input string.


Maybe we don't need to support that:

$wpdb uses mysqli_query

mysqli_query doc page says "mysqli::query -- mysqli_query — Performs a query on the database"

There'e a separate mysqli_multi_query function to run more than one query.

Does this mean WordPress actually doesn't support multiple queries in here?

Also, dbDelta supports multiple queries by exploding the string by ; 🙈 The query parser would make it much safer once we can contribute to core again. cc @dmsnell

Also, it would be useful to have a first-class API for looping over multiple queries in a string – or even in a stream. It's not for this PR for sure, but it seems like we have all the parts that we need. We'd just need to detect when it fails due to a syntax error vs incomplete input and add a method such as append_bytes() and presto. Then we could just run any, arbitrarily large mysql dump.

adamziel · 2025-04-25T15:48:31Z

wp-includes/sqlite-ast/class-wp-sqlite-driver.php

@@ -2182,7 +2236,7 @@ private function translate( $node ): ?string {
 					'%s AS %s',
 					$value,
 					$this->quote_sqlite_identifier(
-						'@@' . ( $type_token ? "$type_token->value." : '' ) . $original_name
+						'@@' . ( $type_token ? "{$type_token->get_value()}." : '' ) . $original_name


nit: slightly clearer code style

Suggested change

'@@' . ( $type_token ? "{$type_token->get_value()}." : '' ) . $original_name

'@@' . ( $type_token ? $type_token->get_value().'' : '' ) . $original_name

adamziel · 2025-04-25T15:52:17Z

wp-includes/sqlite-ast/class-wp-sqlite-driver.php

@@ -3481,10 +3540,9 @@ private function unquote_sqlite_identifier( string $quoted_identifier ): string
 		$first_byte = $quoted_identifier[0] ?? null;
 		if ( '"' === $first_byte || '`' === $first_byte ) {
 			$unquoted = substr( $quoted_identifier, 1, -1 );
-		} else {
-			$unquoted = $quoted_identifier;
+			return str_replace( $first_byte . $first_byte, $first_byte, $unquoted );


Do we need a state machine for this? Or is this actually this easy for all possible scenarios? E.g. quadruple""""quotes, backslashes without \""NO_BACKSLASH_ESCAPES\"" etc.?

adamziel · 2025-04-25T15:55:48Z

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

+				FROM sqlite_schema
+				WHERE type = 'table'
+				AND name != ?
+				AND name NOT LIKE ? ESCAPE '\'


TIL about ESCAPE

adamziel · 2025-04-25T15:58:42Z

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

+	 * @return string[] The names of tables in the information schema.
+	 */
+	private function get_information_schema_table_names(): array {
+		$tables_table = $this->schema_builder->get_table_name( false, 'tables' );


I'm always nervous about having to remember which calls produce a contextually-safe part of the query and which don't. Is there anything we could do to the name to make this clear? As in get_quoted_table_name or get_escaped_table_name() or so?

Alternatively – and for the future – we might want to consider our own flavor of prepared statements that supports binding table names. But that's way beyond the scope here.

Tangentially related, @dmsnell taught me that backticks can be a part of MySQL tables names! So a table can be called wp_plugin_`meta`_data

adamziel · 2025-04-25T16:01:20Z

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

+	 *
+	 * @return array<string, WP_Parser_Node> The WordPress CREATE TABLE statements.
+	 */
+	private function get_wp_create_table_statements(): array {


Nice, so much better than what WordPress core does:

https://github.com/ntwb/wordpress/blob/7b037e5d00fd9fdeed6469dea4f92d47c35739bf/src/wp-admin/includes/upgrade.php#L2509

adamziel · 2025-04-25T16:02:34Z

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

+	private function generate_create_table_statement( string $table_name ): string {
+		// Columns.
+		$columns = $this->driver->execute_sqlite_query(
+			sprintf( 'PRAGMA table_xinfo("%s")', $table_name )


Ditto my point from earlier about safe vs unsafe inputs. Can we either explicitly quote this, even if we don't have to, just to make tell the future reader "yup, all safe, nothing to look for here"?

adamziel · 2025-04-25T16:06:11Z

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

+
+		// Key columns.
+		$key_columns = $this->driver->execute_sqlite_query(
+			'SELECT * FROM pragma_index_info("' . $key_info['name'] . '")'


Double quote is a valid part of a table name and perhaps also a key name in SQLite, let's quote eagerly.

sqlite> create table "tricky_""table""_name" (id int); sqlite> .tables tricky_"table"_name

adamziel · 2025-04-25T16:12:55Z

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

+		}
+
+		// Unquoted string literal. E.g.: abc
+		return $this->escape_mysql_string_literal( $default_value );


Could this happen in practice? Or should this be an error? I'm worried about converting things like DEFAULT NOW() to DEFAULT "NOW()"

adamziel · 2025-04-25T16:13:25Z

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

+		}
+
+		/**
+		 * 5. Otherwise, the affinity is NUMERIC.


great documentation

adamziel · 2025-04-25T16:14:23Z

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

+	private function escape_mysql_string_literal( string $literal ): string {
+		// See: https://www.php.net/manual/en/mysqli.real-escape-string.php
+		return "'" . addcslashes( $literal, "\0\n\r'\"\Z" ) . "'";
+	}


@dmsnell can you poke any holes in this? We assume everything is UTF-8, and if it isn't then GIGO.

adamziel · 2025-04-25T16:15:41Z

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

+	 * @return string                      The quoted identifier value.
+	 */
+	private function quote_sqlite_identifier( string $unquoted_identifier ): string {
+		return '`' . str_replace( '`', '``', $unquoted_identifier ) . '`';


I think you told me once that double backticks are the only way to escape a backtick so this seems fine. Can we mention that in the docstring with a documentation link? It just looks suspiciously easy and I don't want to mislead anyone in the future into searching for a security issue here.

adamziel · 2025-04-25T16:16:37Z

wp-includes/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php

+		$first_byte = $quoted_identifier[0] ?? null;
+		if ( '"' === $first_byte || '`' === $first_byte ) {
+			$unquoted = substr( $quoted_identifier, 1, -1 );
+			return str_replace( $first_byte . $first_byte, $first_byte, $unquoted );


Ditto my state machine question from earlier. Should we use the parser for this?

adamziel · 2025-04-25T16:18:26Z

I left some more notes. We're getting close here. Thank you for persevering @JanJakes! This is the last stretch before we can finally enjoy the new parser everywhere ❤️ I know I'm going deep on a piece of code we only need for a one-off migration, but a) it still matters and b) I think we'll reuse it later on for migrating between the database backends.

Implement a database configurator service for DB initialization and m…

6a641f9

…igration

JanJakes force-pushed the driver-migration branch 2 times, most recently from 6148a0b to 2250ed3 Compare April 15, 2025 15:13

Implement more robust plugin version definition and CI check

12d3063

JanJakes force-pushed the driver-migration branch from 2250ed3 to 12d3063 Compare April 15, 2025 15:26

Load AST classes for tests in test bootstrap

9a6da9d

JanJakes force-pushed the driver-migration branch 4 times, most recently from 7d2c0c1 to 6e94ae8 Compare April 17, 2025 13:42

JanJakes added 3 commits April 17, 2025 16:35

Implement information schema reconstructor

7727cfa

Reconstruct WordPress tables using "wp_get_db_schema" function

23331b3

Avoid race conditions when configuring the SQLite database

ed959fb

JanJakes force-pushed the driver-migration branch from 6e94ae8 to ed959fb Compare April 17, 2025 14:35

JanJakes requested a review from adamziel April 17, 2025 14:51

adamziel reviewed Apr 17, 2025

View reviewed changes

.github/workflows/verify-version.yml Outdated Show resolved Hide resolved

adamziel requested changes Apr 17, 2025

View reviewed changes

JanJakes added 11 commits April 22, 2025 10:13

Improve docs

287f4f4

Fix identifier unquoting

907be69

In WordPress, make sure the "wp_get_db_schema()" function is defined

b178b97

Filter out reserved tables using LIKE

5edd7f7

Improve the clarity of primary key reconstruction

5dddd50

Document SQLite column affinity inline

95b1a19

Do not acquire an exclusive lock for every version check, lock only c…

51b22fa

…onfiguration

Fix $wpdb dependency loop when using schema reconstructor in WP

d904064

Add support for multi-query parsing

69381d8

Store and expose byte start and length in tokens and nodes

cde7da8

Parse queries from "wp_get_db_schema()"

0283884

JanJakes commented Apr 23, 2025

View reviewed changes

wp-includes/parser/class-wp-parser-token.php Show resolved Hide resolved

adamziel reviewed Apr 23, 2025

View reviewed changes

JanJakes added 5 commits April 23, 2025 15:37

Pass input in tokens and get token values lazily

e657ad0

Move multi-query parsing to WP_MySQL_Parser, improve naming and docs

30c8212

Return parser instance rather than a generator

6a3b370

Refactor and document key column definition inference

1121fcc

Refactor and document key definition inference

231bab2

JanJakes force-pushed the driver-migration branch from a8a5604 to 231bab2 Compare April 24, 2025 15:42

JanJakes added 3 commits April 24, 2025 18:09

Improve and document default column value detection

be2841d

Extract WP schema to a method, improve naming and docs

fd55809

For multisite installs, reconstruct WP schema for all sites

baa22c3

JanJakes force-pushed the driver-migration branch from f514c0b to 21476a5 Compare April 25, 2025 10:56

Refactor default value handling, add escaping and tests

c67f6c4

JanJakes force-pushed the driver-migration branch from 21476a5 to c67f6c4 Compare April 25, 2025 11:29

adamziel reviewed Apr 25, 2025

View reviewed changes

JanJakes added 2 commits April 25, 2025 21:53

Fix schema inference for WordPress tables

a1d3b2c

Fix removing schema records for non-existent tables

8bc6652

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Database configuration and migration #42

Database configuration and migration #42

JanJakes commented Apr 15, 2025 •

edited

Loading

adamziel commented Apr 17, 2025 •

edited

Loading

adamziel Apr 25, 2025

adamziel Apr 25, 2025 •

edited

Loading

adamziel Apr 25, 2025

adamziel Apr 25, 2025 •

edited

Loading

adamziel Apr 25, 2025

adamziel Apr 25, 2025 •

edited

Loading

adamziel Apr 25, 2025

adamziel Apr 25, 2025 •

edited

Loading

adamziel Apr 25, 2025

adamziel Apr 25, 2025

adamziel Apr 25, 2025

adamziel Apr 25, 2025

adamziel Apr 25, 2025 •

edited

Loading

adamziel Apr 25, 2025

adamziel commented Apr 25, 2025

	name: Verify that plugin version matches SQLITE_DRIVER_VERSION constant
	name: Assert the WordPress plugin header declares the same version as the SQLITE_DRIVER_VERSION constant

	'@@' . ( $type_token ? "{$type_token->get_value()}." : '' ) . $original_name
	'@@' . ( $type_token ? $type_token->get_value().'' : '' ) . $original_name

Database configuration and migration #42

Are you sure you want to change the base?

Database configuration and migration #42

Conversation

JanJakes commented Apr 15, 2025 • edited Loading

How it works:

adamziel commented Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

adamziel Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented Apr 25, 2025

JanJakes commented Apr 15, 2025 •

edited

Loading

adamziel commented Apr 17, 2025 •

edited

Loading

adamziel Apr 25, 2025 •

edited

Loading

adamziel Apr 25, 2025 •

edited

Loading

adamziel Apr 25, 2025 •

edited

Loading

adamziel Apr 25, 2025 •

edited

Loading

adamziel Apr 25, 2025 •

edited

Loading