The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Follow up of #3586760: Use composite key Upsert queries in core.
Problem/Motivation
There are probably more opportunities to use Upsert with composite key capability in core.
In the Upsert implementation, when the insert fails and the db therefore falls back to an update, we are currently falling back to the minimal case of updating the record with the values passed for insert. However UPSERT can do much more; it can update based an expression. This can be very useful for, for example, ensure an atomic db operation to increment a counter or a score.
Proposed resolution
Extend Upsert to allow specifying column specific expressions for the update case.
Leverage on that to convert all remaining uses of Merge in core, with the exception of StringDatabaseStorage, TaxonomyHooks, Drupal\Core\KeyValueStore\DatabaseStorage.
BONUS: now cache tag invalidations require a single db roundtrip and are atomic.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3604286
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3604286-use-more-composite
changes, plain diff MR !16136
Comments
Comment #3
mondrakeComment #4
mondrakeByproduct: now cache tag invalidations require a single db roundtrip and are atomic.
Comment #5
mondrakeThere's somethiing definitely broken with StringDatabaseStorage, if we try to add a unique index on all the columns of the table but
lidin the update path, we get duplicate key errors.So reverting the changes to that part (which would be questionable anyway due to adding a unique key), and will have to be addressed separately.
Comment #6
mondrakeComment #7
mondrakeGreen on all dbs.
Comment #8
mondrakeWorking on introducing a value object that defines the insert and the update cases in a single point.
Comment #9
mondrakeComment #10
mondrakeComment #11
daffie commentedThe PR looks good!
Just a couple of remarks.
We also need a change record.
Comment #12
mondrakeComment #13
mondrakeReverted controversial changes for now - we can readd later or in follow-ups if needed.
Comment #14
mondrakeAdded draft CR.
Comment #15
daffie commentedIt all looks good to me.
The IS and the CR are also in order.
All the code changes look good to me.
For all changes there is testing added.
For me it is RTBC.
Comment #16
mondrakeComment #17
mondrakeComment #18
mondrakeThere’s a flaw here. Will explain later.
Comment #19
mondrakeThe problem was conceptual - the definition of the update expression was being done as part of specifying the values to be upserted, but we might have multiple set of values in a single upsert operation. In that case, there would be no unique place where to indicate the update expression, that is unique within the upsert. In the latest commit, now that is being done as part of specifying the columns (=fields), that is a one time operation per each upsert call.
Comment #20
mondrakeUpdated CR
Comment #21
daffie commentedLooks good to me.
Back to RTBC.
Comment #22
mondrakeNeed to get #3611653: Upsert fails after upgrade from 11.3 to 11.4 in first.
Comment #23
mondrakerebased and merged with #3611653: Upsert fails after upgrade from 11.3 to 11.4
Comment #24
mondrakeRebased. I had to remove the test that was checking that there was a failure if no columns to be updated were specified, because #3611653: Upsert fails after upgrade from 11.3 to 11.4 actually now supports the 'ignore' case for duplicated keys with no update. I am bold on re-RTBC but a +1 would help I suppose.
Comment #25
mondrakeThe MR here adds an
Expressionclass that would fit nicely with #3586688: Add support for generated columns to the DB Schema API, too. But then the class should better be located in theDrupal\Core\Databasenamespace instead of theDrupal\Core\Database\Queryone. On it.Comment #26
mondrakeDone #25.
Comment #27
daffie commentedLooks good to me.
Back to RTBC.
Comment #28
amateescu commentedReviewed the MR and left some code suggestions. Setting back to NW for the
bindParam()/bindValue()change, which is an actual bug.Comment #29
mondrakeThanks for the review and suggestions. I applied them all.
Comment #30
daffie commentedAll improvements of @amateescu have been applied.
Back to RTBC.
Comment #32
amateescu commentedCommitted and pushed 047e314b510 to main and 7bdc1c89b0f to 11.x. Thanks!