Hydrate from attributes + ids on create() instead of post-insert read#25
Merged
Conversation
The post-insert read in WithDatastoreHandlerMethods::create() races read-replicas on hosts with write/read-split routers (ProxySQL, MaxScale, RDS Proxy, Aurora, Cloudways Autonomous, etc.). The router routes the autocommit INSERT to master and the immediately-following autocommit SELECT to a replica that hasn't yet received the write, producing RecordNotFoundException after a successful insert. Reproduced live on a customer environment: 14/2200 sequential creates failed (~0.6%), all on the same client connection id, all with LAST_INSERT_ID() returning the right id, all recovering within 10-50ms. Wrapping in an explicit transaction eliminated the failure entirely (because routers pin transactional clients to one backend), but adding transactions to every create has its own trade-offs - and the underlying problem shows up on any backend with eventual consistency (Postgres+pgpool, MongoDB read preferences, Cassandra, search indexes, REST APIs behind CDNs). The cleanest fix is to remove the post-insert read entirely. We already have the data the caller passed in, plus the new identity from the insert response. Construct the model from those in PHP and skip the round-trip. What changed: - Column gains an optional withPhpDefault(callable) setter. The callable is invoked at create time for any column the caller didn't provide, so the in-memory model carries the same value the DB would have defaulted in. Backward compatible: existing columns without a phpDefault behave identically. - DateCreatedFactory and DateModifiedFactory now provide phpDefaults that return the current MySQL-format timestamp. The DB-side DEFAULT CURRENT_TIMESTAMP stays in place as belt-and-suspenders coverage for inserts that bypass the framework. - WithDatastoreHandlerMethods::create() applies phpDefaults to the attributes array, runs the insert, and hydrates the model via modelAdapter->toModel( array_merge(attributes, ids)). No second SELECT. Cache pre-warm via the existing cacheItems() so subsequent reads short-circuit through the cache the same way they do today. The new contract: any column whose value should appear in the post-create() model must either be passed by the caller or come from a column factory that provides a phpDefault. Trigger-set values, generated columns, and DB defaults without a matching phpDefault won't be reflected in the in-memory model - they'll need an explicit refresh by the caller if they're load-bearing. Tests cover: - Column.withPhpDefault / getPhpDefault contract - DateCreatedFactory and DateModifiedFactory produce a working phpDefault - create() hydrates without calling query() (no readback) - create() applies phpDefaults for missing columns - Caller-provided values win over phpDefaults Closes the underlying race that motivated phpnomad/wordpress-integration#30 and phpnomad/mysql-db-integration#12 - both can stay closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
WithDatastoreHandlerMethods::create()does anINSERTthen immediately reads the row back to hydrate the returned model. On any host with a write/read-split router in front of MySQL/MariaDB (ProxySQL, MaxScale, RDS Proxy, Aurora, Cloudways Autonomous, etc.), the autocommitINSERTlands on master and the autocommitSELECTis allowed to be routed to a read-replica that hasn't yet received the write. The result is aRecordNotFoundExceptionthrown from a successful insert.This is a property of distributed systems, not of MariaDB specifically. Any future PHPNomad integration against an eventually-consistent backend (Postgres + pgpool, MongoDB with read preferences, Cassandra, Elasticsearch, REST behind a CDN) would exhibit the same failure shape.
Production reproduction
Reproduced on a customer's actual production environment (Cloudways Autonomous, MariaDB 10.11.5 behind a routed K8s service):
baseline(autocommit)txn_per_op(autocommit)big_txn(START TRANSACTION…COMMIT)read_uncommitted(in txn)serializable(in txn)Every failure had the same client-side
CONNECTION_ID(), withLAST_INSERT_ID()returning the correct id, recovering within 10–50ms. The router pins clients to one backend during a transaction (mandatory for txn correctness), which is why every transactional mode passed cleanly.Approach
Most ORMs and platforms (WordPress core, Eloquent, Doctrine, ActiveRecord) avoid this race by not reading back after a write — they construct the model from the data the caller passed plus the new identity, and trust their in-memory representation. This PR brings PHPNomad in line with that pattern.
What changed
Columngains an optionalwithPhpDefault()setterThe callable is invoked at
create()time for any column the caller didn't provide. The DB-sideDEFAULTstays in place as belt-and-suspenders for inserts that bypass the framework.DateCreatedFactoryandDateModifiedFactoryuse itBoth factories now provide a
phpDefaultthat returns the current MySQL-format timestamp. Existing column DDL is unchanged.create()hydrates from attributes + idsNo post-insert
SELECT. The cache pre-warm lands the model under the same key the existinggetModels()flow would have used, so subsequent reads transparently short-circuit through the cache.Contract change
Any column whose value should appear in the post-
create()model must either:phpDefault.Columns relying solely on a DB-side
DEFAULT(without a matchingphpDefault), trigger-set values, and generated columns won't be reflected in the in-memory model. Callers that need those values should refresh explicitly. In practice this is a non-event for existing PHPNomad consumers because all framework-provided default columns flow through the factories updated here.Why this is better than the writer-consistent approach
The previous attempt (#21's predecessor, plus the closed PRs in
phpnomad/wordpress-integration#30andphpnomad/mysql-db-integration#12) wrapped post-write reads in a transaction or used a HyperDB hook to force master routing. That works but:create()even on hosts with no routerHydrating from attributes:
Tests
Column::withPhpDefault/getPhpDefaultcontractDateCreatedFactoryandDateModifiedFactoryproduce a workingphpDefaultcreate()hydrates without callingquery()(no readback)create()appliesphpDefaultsfor missing columnsphpDefaultsPHPStan level 9 clean for the changed files. Pre-existing warnings on
Column.php(about iterable type specs on the original variadic$attributes) are untouched.Related
phpnomad/wordpress-integration#30andphpnomad/mysql-db-integration#12— those defensive PRs can stay closed; this addresses the underlying problem at the right layer.