Add AdapterInterface helpers and replace MySQL-with Postgres - #321
Add AdapterInterface helpers and replace MySQL-with Postgres#321Genaker wants to merge 5 commits into
Conversation
Add AdapterInterface helpers (GROUP_CONCAT, FIELD, casts, CREATE TABLE LIKE) and replace MySQL-only SQL at Magento call sites so a separate Postgres module can implement the same methods without rewriting every query.
Cover MySQL GROUP_CONCAT/FIELD/cast/CREATE TABLE LIKE helpers, TemporaryTableService delegating to createTemporaryTableFromSelect, and BatchIterator keeping minValue when MAX() is NULL. Co-authored-by: Cursor <cursoragent@cursor.com>
Ship the module as genaker/module-postgento. Magento SQL call-site patches are tracked for current cores; after mage-os/mageos-magento2#321 merges they are not needed on Mage-OS.
|
I'm interested in the idea of expanding to Postgres support. I don't see it ever becoming a first-party citizen (something we officially support and encourage use of), especially because so many extensions are likely to be impacted. But if we can shift the core to make it more feasible, cool. As far as this specific PR: To what extent is this tested and confirmed? Have you used it in production? Backwards compatibility is going to be a problem. There are times it's unavoidable, but we want to preserve intercompatibility with Magento Open Source (2.4.x) to the greatest extent possible. The additions to AdapterInterface are a pretty fundamental API change. I'm of split mind on that -- those do belong in the adapter, but BC is a problem. I'm open to discussion/other thoughts on it. I'm also interested in if and to what extent it would be possible for a Postgres adapter to auto-adapt incompatible MySQL code (backticks, etc) to Postgres. Code comments should be reduced. Looks very over the top. |
BulkStatus uses ResourceConnection for getFieldSql. Bundle price, gallery, and status processor tests mock the new adapter methods. PHPCS for ExternalVideo chaining and DbValidator @PARAM.
|
@rhoerr Thanks for looking at this. First-party Postgres. Agreed — this PR does not add a Postgres driver to Mage-OS. It only makes core SQL go through AdapterInterface so a separate module can implement Postgres. The driver is Postgento (Morozov_PgCompat). Same portable-SQL change is open on Magento Open Source: magento/magento2#41129. Extensions that emit raw MySQL stay MySQL-only unless they use the adapter helpers; we are not claiming the extension ecosystem is portable. How far this is tested. I have been running Magento 2.4.8 + Postgento on PostgreSQL 16 in Docker (install, indexers, GraphQL configurable variants, admin, storefront, guest checkout). Magento’s Jenkins on #41129 is green for Integration and WebAPI on MySQL, which is the right check that this PR does not change MySQL behavior. I have not run this as a production merchant store yet. Happy to say that plainly. AdapterInterface / BC. The new methods (getGroupConcatSql, getFieldSql, castToText/castToNumeric, createTableLike, createTemporaryTableFromSelect) are a MAJOR API bump. I put them on the interface because Magento already calls $connection->getIfNullSql() / quoteIdentifier() / insertOnDuplicate() that way — a second, unofficial helper class would be easy to miss and would not type-check. Custom code that extends Pdo\Mysql inherits the MySQL implementations. Code that implements AdapterInterface directly has to add the methods (or we add empty default methods on the interface in PHP 8). I would rather keep them on the interface and document the MAJOR break than hide SQL behind a side API. Open to default methods if that is the BC compromise you want. Mage-OS staying aligned with 2.4.x is why the Magento OS PR exists: once Adobe takes (or rejects) the same helpers, we are not forking dialect APIs. Auto-adapting MySQL SQL. We tried a general rewriter (backticks, IFNULL, GROUP_CONCAT, ON DUPLICATE KEY, …). It is fragile: false positives in string literals, ORDER BY vs identifiers, and it hides the real bug. Postgento does not rewrite arbitrary SQL. It implements the same helpers, quotes identifiers, and keeps only two leftover rewrites that still need a live connection (SHOW TABLES / SHOW TRIGGERS, and raw ON DUPLICATE KEY text). Backticks in core are meant to become quoteIdentifier() in this PR, not regex. Third-party SELECT \sku`` will still fail on Postgres unless they fix call sites or we add a rewriter people can opt into — I would keep that out of core. Comments. Fair. I will strip the long “PgCompat / why Postgres” comments down to Magento-style notes only where the SQL change is non-obvious (e.g. GROUP BY + extra selected columns). The Magento OS PR already had those comments removed. Happy to split “interface methods + Mysql implementations” vs “call-site SQL” if that is easier to review. |
|
Coding standard and Sansec are green. The unit failures are not from this PR — we don't touch those tests:
Same for Weee integration: Happy to split or follow up if you want those harness issues fixed on main instead of here. |
|
Thanks, will dig in deeper when time allows |
Description (*)
Magento core still embeds MySQL-only SQL (
GROUP_CONCAT(),FIELD(),IFNULL()literals, backticks,CREATE TABLE … LIKE,CREATE TEMPORARY TABLE … ENGINE=… IGNORE (SELECT),ON DUPLICATE KEY UPDATEtext,UNIONof mixed types,UPDATE … LIMIT,SHOW VARIABLES LIKE 'version'). That is valid on MySQL and fatal on PostgreSQL.This PR does not add a Postgres adapter. It adds dialect helpers on
AdapterInterface/Pdo\Mysqland switches Magento call sites to those helpers (and to existing ones such asgetIfNullSql()/quoteIdentifier()/insertOnDuplicate()).The Postgres driver lives in a separate module: https://github.com/Genaker/postgento (fork of Kirill Morozov’s
Morozov_PgCompat). That repo implements the sameAdapterInterfacemethods onpdo_pgsql. Mage-OS can review/test/merge this core PR on MySQL alone, and optionally install Postgento on a branch of this PR to run Magento on PostgreSQL. After this PR is in Mage-OS, Postgento does not need the Magento SQL call-site patches.New adapter methods (MySQL implementations):
getGroupConcatSql()→GROUP_CONCATgetFieldSql()→FIELD()castToText()/castToNumeric()(no-op /CAST(… AS DECIMAL)on MySQL; other engines can map to::text/::numeric)createTableLike()→CREATE TABLE … LIKEcreateTemporaryTableFromSelect()→ existing MySQLCREATE TEMPORARY TABLE … ENGINE=innodb IGNORE (SELECT)Also:
Selectconstructor type-hint relaxed fromPdo\MysqltoZend_Db_Adapter_Abstractso a non-MySQLAdapterInterfacecan callselect().--db-engineis passed throughDbValidator/Installer::assertDbAccessible()so an optional engine can open the correct driver at install time. Default MySQL behavior is unchanged.Behavior on MySQL is intended to be equivalent.
Related Pull Requests
Postgres adapter (not in this repo): https://github.com/Genaker/postgento — please test or merge that module alongside this PR if you want a full Postgres install.
Fixed Issues (if relevant)
N/A — no Mage-OS issue filed yet.
Manual testing scenarios (*)
On a MySQL Mage-OS install (this PR must not require Postgres):
bin/magento setup:upgrade— exit 0, no SQLSTATE.bin/magento indexer:reindex— all indexers complete (catalog price, stock, EAV, catalog rule, fulltext).WJ01) returns variants;GROUP_CONCAT/FIELD()call sites used by collection / search appliers.setup:install --db-engine=mysqlstill validates and installs as today.Optional Postgres: check out this PR, Composer-require https://github.com/Genaker/postgento (
genaker/module-postgento),setup:install --db-engine=postgresql, then repeat indexer / GraphQL / storefront / admin / checkout.Questions or comments
Happy to split adapter-interface methods vs call-site SQL into two PRs if that is easier to review. The adapter itself is Postgento — Mage-OS can merge this PR first (MySQL-safe) and take Postgento as a Composer module (or Mage-OS Lab) whenever you want Postgres in the distribution.
Unit tests for the new
Pdo\Mysqlhelpers,TemporaryTableService, andBatchIteratorempty-batchminValueare in this PR.Contribution checklist (*)