Driver parity

[JanJakes]

This pull request implements additional MySQL statements in the new driver and adds various fixes for better parity of the new SQLite driver with the old one. It also ports some smaller test files that I missed in the original PR.

The following MySQL statements are implemented:

• SHOW COLUMNS
• TRUNCATE TABLE
• ANALYZE TABLE
• CHECK TABLE
• OPTIMIZE TABLE
• REPAIR TABLE
• Access to information_schema.
• USE information_schema & USE

The following additional improvements are implemented:

• The reserved prefix for internal tables and other objects is now protected from direct use.
• The information schema can appear only in read-only queries.
• More correct data in information schema.
• Some grammar fixes.
• More tests.

Related issues:

• Implements Support querying information_schema tables #9.

[adamziel]

We don’t actually support the nuances of myisam vs innodb, right? This test is great btw, I just want to verify my own understanding

[JanJakes]

@adamziel That's correct. We don't support any of these nuances (yet?), but we save the exact definition in the information schema, so things like SHOW CREATE TABLE reflect that.

[adamziel]

According to SO:

The main differences between InnoDB and MyISAM ("with respect to designing a table or database" you asked about) are support for "referential integrity" and "transactions".

We choose InnoDB if we need the database to enforce foreign key constraints or support transactions (i.e. changes made by two or more DML operations handled as single unit of work, with all of the changes either applied, or all the changes reverted). These features are not supported by the MyISAM engine.

Another big difference is concurrency. With MyISAM, a DML statement will obtain an exclusive lock on the table, and while that lock is held, no other session can perform a SELECT or a DML operation on the table.

AFAIR WordPress can use either for core tables and some hosts enforce a specific choice. We can support transactions, strong constraints, and row-level locking and you typically expect transactions to work so I'd say we should support InnoDB and log a warning when we see MyISAM in a create table query. These warnings will quickly get annoying so we could add a driver-level warning control with an option such as "warnings" => array( "db_engine" => true|false, ... )

[JanJakes]

@adamziel Good point! I created a ticket: #24

[adamziel]

Do we support different collations? Or is it always the same underlying collation?

[adamziel]

I wonder if we should throw an error on collation mismatch, or at least a warning. I guess some apps will try to use collation that’s not supported in sqlite, but it won’t really matter in at least some of them. A warning perhaps? Or a wp amin notice?

[JanJakes]

@adamziel At the moment, we don't apply collations in any way, so internally, our encoding is utf8mb4 and the collation probably something like utf8mb4_unicode_ci, utf8mb4_general_ci, or utf8mb4_0900_ai_ci (I need to check what exactly happens in SQLite and which collations they support).

I wonder if we should throw an error on collation mismatch, or at least a warning.

Good question. I think it's tricky to be strict here because even though WP Core embraced utf8mb4, some plugins may still have not done that properly, even though they could be saving Unicode bytes in some columns. It seems to me like this may need a bit more investigation. Maybe I should create a ticket?

[adamziel]

Yes, let's create a ticket. As for the next steps, I'd suggest:

• Throwing an exception if we see a collation referring to a different character set or comparison rules
• Adding an option like enforce_utf8_charset that defaults to false for all the other cases. When we see a mismatched encoding and the option is true, we'd log a warning and use a sensible default encoding when we see a query with a "salvageable" encoding or collation definition. When the option is false, we'd throw a fatal error explaining what happened and that there's an option you can use. It could also be called strict_mode or so.

What's "salvageable" is quite arbitrary and it's easier to say what isn't.

For example, an incompatible set of collation rule would lead to a very different application behavior and I'd just throw an error right away. MySQL Collation doc page explains different parts of the collation suffix:

Suffix | Meaning
-- | --
_ai | Accent-insensitive
_as | Accent-sensitive
_ci | Case-insensitive
_cs | Case-sensitive
_ks | Kana-sensitive
_bin | Binary

When a table declares latin1, utf16, or anything that isn't utf8, we'd likely break the app by quietly using utf-8.

On the flip side, I think we're good to rewrite utf8, utf8mb3, and other similar variations as utf8mb4 (when the option is set). Unicode characters sets page lists deprecated character sets and recommends using utf8mb4 instead. That would still change the application behavior, but I can't imagine a plugin that relies on collating up to 3 bytes from every UTF-8 characters and not the fourth byte. Similarly, utf8mb4_general_ci is deprecated in favor of utf8mb4_unicode_ci and I think we could treat them as the same charset.

The Unicode character sets page also discusses other variations, such as general_, language-specific character sets, etc. It's important that we're aware of this general problem space and, when in doubt, default to throwing an error instead of continuing silently.

[adamziel]

I don't mean it as a blocker for this PR, by the way. We'd just need to add, at a very least, the exception throwing in the very next PR to avoid creating an illusion of a working application.

[JanJakes]

@adamziel I created the ticket: #25

I'm curious which of the collations and suffixes we'll be able to correctly support in SQLite. SQLite has some collation support, but I haven't explored that yet.

Once we start throwing the exception, I'm also curious if it will cause any Playground Tester test cases to fail.

[adamziel]

Since these are not valid SQL queries, let's mention the <prefix> idea in a docstring just to save our future selves some confusion

[JanJakes]

👍 Adding comments.

[adamziel]

We could handle it by creating a new $slugified_db_name.sqlite file. For now, let's throw a clear error. We should not see a CREATE DATABASE query in practice, but if some plugin uses it we'll need to know.

[JanJakes]

👍 At the moment, it will explicitly fail (fall trough to the default case), but I'll update the comment to add your idea.

[JanJakes]

The reason I added this comment is that the old driver implements this as a no-op. Now, we're stricter, so let's see what it does in the test suite.

[adamziel]

Good, let's block any writes to information_schema.

[adamziel]

Is there any way this identified would show up before we have a chance to set is_readonly? If yes, we may need to move this check before the query execution once we have all the information.

[JanJakes]

@adamziel Not at the moment. The $this->is_readonly = true is set only on the top-level in the execute_some_type_of_statement methods. It's actually only SELECT, SHOW, DESCRIBE/EXPLAIN at the moment.

[adamziel]

I left a few notes about database engines and character sets that will matter in a very short term, but we don't need to solve them for the purposes of Driver parity. The old driver didn't handle either. This PR looks great, thank you so much @JanJakes

[adamziel]

With this PR, we could make the new driver the default, no?

[JanJakes]

@adamziel I created the tickets and added two commits with improved comments.

With this PR, we could make the new driver the default, no?

For Playground, I think so.
For Studio, we'll need to address the migration (~ export & import) first, or make sure it's used for new sites only.

That said, I'd like to first get the Playground Tester reports up-to-date after this is merged. Without them, I don't have the confidence we won't break some plugins in a way that is not covered by the unit tests.

[JanJakes]

@adamziel Since this was already approved, I'll merge it now, and if you have any more suggestions, I'll create tickets for them.