Upsert fails after upgrade from 11.3 to 11.4

git log --oneline 11.3.13..11.4.1|grep -iE "(upsert|database)"
a3e92473065 revert: #3605262 Database write optimizations result in inconsistent data in data and revision tables
c8c3972c9f1 task: #3586760 Use composite key Upsert queries in core
c648adcfce9 perf: #3588385 Add a transaction around the database key value store setMultiple()
72095f8d09c fix: #3588702 Install mysqli in test 11.3 database fixture
529ca79cc1d perf: #3561960 Combine the database queries in ::loadFromSharedTables() and ::loadFromDedicatedTables()
77799e3eb8d task: #3581442 Replace usage of uniqid() in the Database system
2e8705a4bb1 feat: #2547493 Add support for unique / primary key constraints composed of multiple fields for Upsert queries
d344795bfed Revert "task: #3569127 Add new 11.3.x database dump fixtures, without modules deprecated for removal in 12.x"
3d860a3c532 task: #3569127 Add new 11.3.x database dump fixtures, without modules deprecated for removal in 12.x
06d89e94aeb task: #3569127 Add new 11.3.x database dump fixtures, without modules deprecated for removal in 12.x
d9f4bb9c658 fix: #2722307 Move translation based conditions into database query on revisions overview page
c840c458bc3 perf: #3564689 Combine multiple cardinality field loading into a single database query
1f71e2efe92 perf: #3493290 Try to reduce the number of database queries in MenuTreeStorage::rebuild()
cbb23a2aebd fix: #3561800 Using \Drupal\Core\Database\Query\Insert::from() on postgres on a table with a serial field can result in duplicate key error
1fe6aeaf13e task: #3554579 Optimize database writes when re-saving a pending revision as the default one
673fd32689e task: #3549727 Fix LongLineDeclaration in module Controller, Hook and Database directories
223992a21c0 perf: #3551308 Combine single cardinality fields into a single database query when loading entities

#2547493: Add support for unique / primary key constraints composed of multiple fields for Upsert queries and #3586760: Use composite key Upsert queries in core actually changed upsert, so I would start there.

The error is SQL syntax, not duplicated key: nothing is specified after ON DUPLICATE KEY UPDATE in the SQL statement string. This happens if the 'key' column(s) correspond with the 'field' column(s). To prove that you may try to patch with #3604286: Upsert - Allow to customize the behavior of the update when the insert fails and use more Upsert queries in core, that adds a specific \LogicException('There must be at least one column to update in an upsert operation') error in that case.

#13 then using upsert in this case is nonsense, because the key columns, being duplicated, are not changing and there are not columns to update.

It's in fact an insert that should keep going in case of IntegrityConstraintViolationException, then.

But if an upsert was abused for this, let's see if we can fix it not to break things. Maybe we just deprecate the degenerate case for now and throw the exception in a later major.

Ah but then it's a multi-value insert, i.e. in the same statement there is input for multiple rows to be inserted, and failure of one will fail the whole batch. We certainly need a test first of all, as coverage of that scenario is missing; and we need to see that it can work on all the dbs.

I still believe that using upsert in this case is not efficient, as it will force an useless additional update on all the rows where the key is already present. So directionally IMHO regardless of how we end up here, we should deprecate supporting the degenerate case.

luckily all dbs have syntax that covers this case natively

#24 yeah in the end if there's native support and with that we can avoid db roundtrips for multiple inserts in favor of a single SQL statement, we might just call this a supported case. Even if core is not using it at the moment (and that's why it got missed), now there's test coverage that would prevent regressions. So I retract the proposal to deprecate.

Asked AI for a review, and it suggested adding a test for the composite key degenerate case, which I think is a good idea.

Added testDegeneratedCompositeKeyUpsert. AI assisted in the making.

all green now

Small tuning of the comments. Leaving at RTBC.

Following the discussion I was having similar reactions to @mondrake including this one:

yeah in the end if there's native support and with that we can avoid db roundtrips for multiple inserts in favor of a single SQL statement, we might just call this a supported case

The changes here aren't necessarily easy to follow but I don't think the resulting logic is particularly more difficult than it was before, and we'd need to detect the case to throw an exception, so seems OK to leave it supported as long as it's not causing problems for other cases.

Committed/pushed to main, cherry-picked to 11.x and 11.4.x, thanks!