-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Respect strict/non-strict MySQL modes #29
Conversation
a70adb8
to
bbb6e70
Compare
7107671
to
3939896
Compare
The approach taken here is a good compromise. I'm sorry none of the other paths didn't work. One thing that bothers me is the proliferation of ways to insert and update data in MySQL. Let's make sure all our bases are covered – here's a few more query types I can think of:
We don't have to actually support all of these on day 1, but it would be useful to at least display a warning when running into an unsupported combination of operation + sql mode. |
Would this also support things like Also, there's at least a few scenarios where we could reasonably bale out, e.g. most hosts would likely not allow a SET that updates |
* See: | ||
* https://dev.mysql.com/doc/refman/8.4/en/data-type-defaults.html#data-type-defaults-implicit | ||
*/ | ||
const DATA_TYPE_IMPLICIT_DEFAULT_MAP = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really good find
} | ||
} | ||
|
||
// TODO: Support user variables (in-memory or a temporary table). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's communicate lack of support with a warning or error to at least make the developer aware why their code breaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Added in d6f6b6d.
} | ||
} | ||
|
||
// TODO: Handle GLOBAL, PERSIST, and PERSIST_ONLY types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's bale out with an error to make sure we don't give a false illusion of these queries actually working. It should also make it easier for us to spot any plugins relying on these semantics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Resolved in d6f6b6d.
@@ -2592,6 +2877,203 @@ private function translate_show_like_or_where_condition( WP_Parser_Node $like_or | |||
return ''; | |||
} | |||
|
|||
/** | |||
* Translate INSERT body, emulating MySQL implicit defaults in non-strict mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great comment ❤️ it makes the intended purpose very clear 🚀 I want to highlighting that for everyone, cc @bgrgicak @brandonpayton @akirk @zaerl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I'd add is the INSERT before the translation and the expected INSERT after the translation – same as in the PR description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Added the input/output in f922251.
$select_list[] = $stmt->getColumnMeta( $i )['name']; | ||
} | ||
} else { | ||
// When inserting from a VALUES list, SQLite uses "columnN" naming. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a few nitpicks around being very strict with errors in cases we don't support. The PR is solid, the approach well-researched, and the reasoning clear. It's always a pleasure to review these SQL PRs, thank you for your great work @JanJakes !
@adamziel Thanks!
Probably not, these seem to be part of the expression grammar, and not part of the
True, we'll need to go through a list of the variables at some point. |
3939896
to
d39df53
Compare
Great point to check whether all ways of data manipulation are covered or disabled!
Can a This made me think if there are any other variants for the I found one I didn't consider before — SET SESSION sql_mode = '';
CREATE TABLE t1 (id INT PRIMARY KEY, name TEXT NOT NULL);
INSERT INTO t1 (id) VALUES (1);
-- this works (implicit default)
UPDATE t1 SET name = NULL;
-- this fails with "ERROR 1048 (23000) at line 6: Column 'name' cannot be null"
INSERT INTO t1 (id) VALUES (1) ON DUPLICATE KEY UPDATE name = NULL; So that's great, since it already behaves that way, and I only added a test. |
Oh MySQL 🙈 |
try { | ||
$this->execute_sqlite_query( 'DELETE FROM sqlite_sequence WHERE name = ?', array( $table_name ) ); | ||
} catch ( PDOException $e ) { | ||
if ( str_contains( $e->getMessage(), 'no such table' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would checking the error code be more reliable here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel For some reason, I'm not getting the correct SQLSTATE back: SQLSTATE[HY000]: General error: 1 no such table: sqlite_sequence
It may be something about the PDO driver in SQLite. The correct SQLSTATE should be 42S02
.
* Rewrites a statement body in the following form: | ||
* INSERT INTO table (optionally some columns) <select-or-values> | ||
* To a statement body with the following structure: | ||
* INSERT INTO table (all table columns) | ||
* SELECT <non-strict-mode-adjusted-values> FROM (<select-or-values>) WHERE true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
// 4. Compose a new INSERT field list with all columns from the table. | ||
$fragment = '('; | ||
foreach ( $columns as $i => $column ) { | ||
$fragment .= $i > 0 ? ', ' : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing actionable for this PR but man, I wish we had a more convenient way of building these SQL queries – both for us here and for WordPress core. cc @dmsnell – we've discussed this recently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel Just yesterday, I was trying to brainstorm with ChatGPT about a lightweight but powerful SQL builder API in PHP. PHP is quite limited for these types of APIs, especially with typing (we'll never be able to do anything like Drizzle ORM), but it would definitely be great to have something like that for WordPress.
$values = 'insertFromConstructor' === $node->rule_name | ||
? $node->get_first_child_node( 'insertValues' ) | ||
: $node->get_first_child_node( 'queryExpressionOrParens' ); | ||
$fragment .= ' FROM (' . $this->translate( $values ) . ') WHERE true'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need WHERE true? Is this stitched with AND ...
later on? Also, TIL we can do a subquery like that without giving it an alias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel It was failing without that, and then I found it documented:

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL. Let's leave a comment with a link inline – it's the kind of thing that seems too easy to remove when not careful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Adam Zieliński <[email protected]>
Summary
This PR implements the emulation of MySQL no-strict mode (disabled
STRICT_TRANS_TABLES
/STRICT_ALL_TABLES
) in SQLite, and its modification and retrieval usingSET
/SELECT
queries.The non-strict mode is a requirement for WPDB, but in other environments (phpMyAdmin, etc.), it's likely to be left to the MySQL default, which is
STRICT_TRANS_TABLES
(strict mode forInnoDB
tables).Closes #17.
Strict vs. non-strict mode
The implementation was done in the PHP SQLite driver code, and from a high-level perspective, it does the following:
INSERT INTO table (optionally some columns) <select-or-values>
is rewritten toINSERT INTO table (all table columns) SELECT <non-strict-mode-adjusted-values> FROM (<select-or-values>)
.UPDATE table SET column = <value>
is rewritten toUPDATE table SET column = COALESCE(<value>, <implicit-default>)
, when the column is non-nullable.Details
When zooming into the details, the above-described logic is quite involved and handles things like:
INSERT
statements may list no fields, or any fields in any order.INSERT INTO ... SELECT ...
we don't know what fields theSELECT
returns, and we need to figure that out.UPDATE
statements, aNULL
may set aNULL
value or an implicit default, depending on the column definition.SET/SELECT SQL mode
The PR also implements the following statements:
SET
for sessionsql_mode
value.SELECT
for sessionsql_mode
value.The
SET
statement is a surprisingly complex one, with various syntax flavors and multi-command support. Statements likeSET @var = '...', SESSION sql_mode = '...', @@GLOBAL.time_zone = '...', @@debug = '...', ...
are valid, including strange semantics, such as carrying the variable type keywords (SESSION
, etc.) through to the subsequent definitions in the same statement.WPDB
This PR also handles
$wpdb->set_sql_mode()
for the SQLite driver, as well as initializing the SQL mode for each connection.Strict/non-strict behavior
Here's a summary of the implemented strict vs. non-strict mode behavior:
Other paths considered and attempted
Initially, I started the implementation with the presumption that handling the strict vs. non-strict mode correctly and dynamically on the PHP driver side would be very hard, and I tried to implement this using other approaches at first.
1.
DEFAULT
andCHECK
expressionsSQLite supports expressions in
DEFAULT
definitions for columns, as well as inCHECK
constraints. The natural first direction would be to make a default conditional to emulate the non-strict mode, or, conversely, make a constraint check conditional to emulate the strict mode.It seems elegant in theory, but it doesn't work because both
DEFAULT
andCHECK
expressions need to be "constant", which means they can't reference any other columns or tables, and they can only use deterministic functions, that is, functions that always return the same value during a single session (except for date/time functions that are a special case).This means using these expressions to apply strict/non-strict mode conditionally is not possible (and it's actually not possible to use non-deterministic user-defined functions in other cases either).
2. Triggers
The idea here was to use before or after
INSERT
andUPDATE
triggers to modify the new column values to emulate the MySQL implicit defaults. For each table, a specifically tailored trigger could be composed at the time of the table creation or modification.This turned out to be problematic, and actually nearly impossible without additional complex or dirty hacks because:
NEW
row values in triggers. We can overcome this by usingAFTER
triggers and updating the values.NULL
value from an omitted value fromINSERT
within a view. This is a dealbreaker, although solvable with views (see below).application_id
) are not session-specific. Using a real table for this seemed to become a bit too hacky to me. However, this can be overcome with views (see below).The prototype can be found in the following draft PR: #30
3. Views & triggers
SQLite supports views, and even though they are not writable, we can use
INSTEAD OF
triggers to hijack the values from theINSERT
orUPDATE
query, and perform custom logic instead. This approach is very similar to the previous one with triggers only, and it solves its main pain points:INSERT
orUPDATE
against the original table or against a view that implements "non-strict" triggers.NULL
value from an omitted one can be done with a hack. The view can define an extra column with a default value, and we can then use that column to sneak in a string of column names that were explicitly used in the insert statement. It's not a nice solution, but it works.This approach seemed to work, until I executed the full test case only to realize that
SQLITE_ERROR: sqlite3 result code 1: cannot UPSERT a view
. So that was it for views.The prototype can be found in the following draft PR: #31