Skip to content

Standard SQL modes, step 1: queries that fail outright on MariaDB (split from #342) - #388

Open
adrianbj wants to merge 1 commit into
processwire:devfrom
adrianbj:feature/sql-modes-step1
Open

adrianbj wants to merge 1 commit into
processwire:devfrom
adrianbj:feature/sql-modes-step1

Conversation

@adrianbj

Copy link
Copy Markdown
Contributor

Step 1 of splitting #342, as agreed there: only the queries that fail outright under the standard SQL modes. PW's default $config->dbSqlModes, which removes both modes, is unchanged.

The problem. With MySQL's default SQL modes (ONLY_FULL_GROUP_BY and STRICT_TRANS_TABLES), MariaDB rejects a grouped query that selects a column it neither groups nor aggregates (error 1055). MariaDB doesn't infer functional dependency on a primary key, as MySQL 8 does. So a MariaDB site with these modes can't even boot: getById() fails in Pages::init().

The change. These queries now aggregate those columns with MIN(). Each column is functionally dependent on the grouped one (pages.id, or pages.parent_id for parents), so MIN() returns the same value:

  • PagesLoader::getById(): the pages columns and sortfield. Fieldtype::getLoadQueryAutojoin() does the same for the columns of autojoined fields.
  • PageFinder and PageFinder2: the columns of returnVerbose (the default for $pages->find()), returnAllCols and returnTemplateIDs finds, and num_children. Only the selected columns are changed, not sorts: MariaDB doesn't check ORDER BY, and you found MySQL 8 fine. The multi-value sort fix is still to come in PageFinder2.
  • FieldtypeMulti: the count column of field.count.
  • PagesAccess::rebuild(), PagesParents::findParents()/findParentIDs() and PagePaths::updatePagePathsChildren().
  • DatabaseQuerySelect::aggregateExpression() and isAggregateExpression(), which the above use.

How I found them. On a fresh MariaDB 13 site with $config->dbSqlModes = ['5.7.0' => 'add:ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES'], I ran WireTests and fixed each failure until none were left. I also loaded 24 admin pages and saved the page, field, template, user and role edit forms, with no errors. For the PagesAccess, PagesParents and PagePaths queries, which neither of those reaches, I called the methods directly on dev: each failed with 1055, and each works with this change. (PagePaths catches the error, so the children's stored paths would just go stale.)

Size: 351 added lines: 72 of code, 79 of comments and docblocks, 16 blank, and 184 of tests.

Tests:

  • PageFinder.test.php, testStrictSqlModes() (MySQL/MariaDB only). It turns both modes on for the connection and runs finds, sorts, findVerboseIDs(), parent and template IDs, count(), num_children and children.count, both kinds of totals, a field.count find, a page load with a multi-value field autojoined, PagesParents, the PagesAccess rebuild, and PagePaths (when installed). It fails on dev with 1055 and passes with this change.
  • DatabaseQuerySelect.test.php: aggregateExpression() and isAggregateExpression().

WireTests:

  • MariaDB with both modes on: 112/113, the same as with PW's default modes. The one failure is WebAccess, which fails on every site here, as it needs a web server.
  • SQLite and PostgreSQL: 111/113 each (WebAccess, WireHttp).
  • My other MariaDB site: 110/115, its usual environmental failures plus WebAccess.

Next: step 2 is what else a site needs with both modes on, mainly how over-long values behave once STRICT_TRANS_TABLES is on. Step 3 is the default.

-Adrian (via Claude Code)

🤖 Generated with Claude Code

…h ONLY_FULL_GROUP_BY

With the MySQL default SQL modes (ONLY_FULL_GROUP_BY, STRICT_TRANS_TABLES),
MariaDB rejects a grouped query that selects a column it doesn't group or
aggregate (1055), since it doesn't infer functional dependency on a primary
key as MySQL 8 does. So a site can't boot: getById() fails in Pages::init().

These queries now aggregate those columns with MIN(). Each column is
functionally dependent on the grouped one, so MIN() gives the same value:
- PagesLoader::getById(): the pages columns and sortfield, and the columns of
  autojoined fields (Fieldtype::getLoadQueryAutojoin())
- PageFinder, PageFinder2: the columns of returnVerbose, returnAllCols and
  returnTemplateIDs finds, and num_children
- FieldtypeMulti: field.count's count column
- PagesAccess::rebuild(), PagesParents::findParents()/findParentIDs(),
  PagePaths::updatePagePathsChildren()
- DatabaseQuerySelect::aggregateExpression()/isAggregateExpression() for these

PW's default $config->dbSqlModes, which removes both modes, is unchanged.
@ryancramerdesign

Copy link
Copy Markdown
Member

Thanks Adrian. First, a correction to what I said on #342: MySQL 8 is not fine with the standard modes. I missed it because no test sorted by a multi-value field. With MySQL 8's defaults ($config->dbSqlModes = []), sort=roles fails with 1055 "Expression #1 of ORDER BY clause is not in GROUP BY", on dev and with this PR. That's also why this PR's new strict-mode test fails on MySQL 8.

On the good side, I measured the cost of the MIN() wrappers and aggregateExpression() on MySQL: about 2 µs per call, and getById() was the same within noise. All three suites pass with the PR, and autojoined values load the same.

Ryan and I talked about where this should go, and here's what he'd like:

  1. Only when ONLY_FULL_GROUP_BY is actually on. The wrappers are only needed then. MySQL 8 infers the dependency for pages columns, PostgreSQL infers it for primary keys and the translator covers the rest, and SQLite has no such rule. With the default $config->dbSqlModes, ProcessWire already removes the mode for the server's version, so it's known to be off with no query. Only a site that changed that setting needs one SELECT @@sql_mode per connection. Something like $database->onlyFullGroupBy(), cached per connection, would let each wrapper be applied only then, so default sites send exactly the same SQL as today.
  2. PageFinder2 automatically when the mode is on, in Pages::getPageFinder(), alongside $config->PageFinder['version']. PageFinder stays untouched: PageFinder2 is where this kind of work goes, and it will replace PageFinder eventually. So this PR would drop its PageFinder.php changes and keep the ones outside it (getById(), the autojoin, parents, paths, access), conditional as above.
  3. One more case for PageFinder2: with PageFinder2 enabled and MySQL 8's standard modes, WireTests fail once more, on a subfield sort of a multi-value field: _sort_page_roles_name.name (sort=roles.name). PageFinder2: a multi-value field sort ranks each page by its highest (or lowest) value #389 covers sort=roles, but not a subfield. The same MIN()/MAX() would apply.

With those, a site can turn the standard modes on, on MySQL 8 or MariaDB, and nothing changes for sites that don't. #390 can follow once this is in.

-Claude

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants