Add support for unique / primary key constraints composed of multiple fields for Upsert queries

Patch looks good.

1. The testbot is failing for PostgreSQL.

2. +++ b/core/lib/Drupal/Core/Database/Query/Upsert.php
   @@ -47,13 +47,47 @@ public function __construct(Connection $connection, $table, array $options = [])
   +    $this->fields($fields);
   

   Why do we do a call to the method fields()? We are not doing that in the method key().

3. +++ b/core/lib/Drupal/Core/Database/Query/Upsert.php
   @@ -47,13 +47,47 @@ public function __construct(Connection $connection, $table, array $options = [])
   +    if (empty($values)) {
   +      if (!is_numeric(key($fields))) {
   

   Can we combine the 2 if-statements in a single if-statement.

4. Can we use the upsert also for Drupal\Core\KeyValueStore\DatabaseStorage::setIfNotExists()?
5. Can we replace all calls to Drupal\Core\Database\Query\Merge? If so, can we deprecate Merge?
6. Missing testing for upserts with the methods fields() with the parameter $values being not empty.

7. +++ b/core/lib/Drupal/Core/Database/Query/Upsert.php
   @@ -47,13 +47,47 @@ public function __construct(Connection $connection, $table, array $options = [])
   +   * @param array $fields
   ...
   +  public function keys(array $fields) {
   

   The parameter $fields, is that only an array with values or can it also be an array with keys and values?

8. Can we do some performance testing to make sure that with this patch Drupal becomes faster.

@amateescu: Thank you for your work on this.

Unfortunately it no longer applies to 9.5.x or 10.x. Any chance you have rerolled this patch recently?

Stripped down, MySQL only, no tests, try at an updated patch. Sorry will try to do more. Though i do think the adding the upsert multivalue capability and using elsewhere in Drupal can and should be separated issues.

what are the odds this could be a breaking change for a contrib module?

This looks good, agreed we should leave any deprecation of ::merge() for a separate issue, but maybe good to open that?

I guess my only question would be what the chances are of a third party driver that can't handle this, but they could probably fall back to a merge query or something?

Erm what.

1. With keys added you are now at a point where MERGE and UPSERT functionalities overlap so much that MERGE can just fall back to an upsert if the more complex methods are not called.
2. As far as I can see with ag -lic upsert |grep -v ests/ there's not a single use of upsert in core. Merge is used widely. Personal anecdote: I needed to check git to see when upsert was added despite I am credited on the original issue from 2015. I am fairly sure I never used it.
3. Why on earth would you drop MERGE when support is arising finally? In the Merge class doxygen I wrote "Other databases (most notably MySQL, PostgreSQL and SQLite) will emulate this statement by running a SELECT and then INSERT or UPDATE" so many years ago when life was worth living but by now this is not true as PostgreSQL 15 in 2022 added MERGE support -- and D11 requires 16. Documentation and code both should be upgraded accordingly.

tl;dr: drop UPSERT, make MERGE smart, implement full native MERGE, use it for PostgreSQL for now, rejoice. (Oracle and SQL Server drivers will pick up the native MERGE, too. Let's hope one day M{y,aria}SQL will too.)

I'm not sure we necessarily have to get rid of either MERGE or UPSERT - googled a bit and it looks like MERGE is the ANSI standard, whereas UPSERT can be considered an implementation of MERGE with a specific purpose of "update if exists, insert if not exists". Which AFAICS is what Drupal is in need of when currently using MERGE. For the time being, I'd suggest moving ahead here as a generalization of UPSERT, while keeping MERGE for now.

I'd not add a separate ::keys() method here. Rather overload the existing ::key() method to accept string|array. And maybe deprecate passing a string. Or introduce a ::primaryKeyColumns() method to make clear you need to pass all the PK columns to the query.

UPSERT queries can work with unique keys as well (as documented by the current MR), and generalizing "unique keys" and "primary keys" brings us back to the keys() method.

I thought about this. TBH I find a bit confusing that, if you have a primary key (which I think it's the case of 100% tables in Drupal today?), you could UPSERT based on a different unique key - because the upsert may try to update or insert a primary key that would fail uniqueness. In other words it seems to me allowing to shoot oneself's foot. But maybe it's not for here.

How about this?

• add a ::keyColumns(array $columns) method the array being list<string>; so we don't get into primary vs unique debate but we clearly say we need a list of columns (that could be a list with one element only, of course)
• deprecate the ::key(string $field) method on behalf of ::keyColumns()

Left comments inline. We also need a deprecation test showing that the upsert using ::key() still works. The PostgreSQL test failures seem legit.

Updated MR target branch from 11.x to main branch.

Here's the thing:

1. You can't drop MERGE because it supports vastly more than UPSERT. And, again, having support for that is great because finally ANSI 2003 implementation have started to emerge. (There was no need to Google this, there's a mile long comment in the Merge class doxygen towards this.)
2. Having UPSERT do the same as MERGE means there's more maintenance and more confusion from developers

The original UPSERT issue had this reasoning:

Currently we implement a subset of the ANSI SQL:2003 MERGE command but another subset is desirable, one that SQLite implements with INSERT ON CONFLICT REPLACE, MySQL implements with REPLACE or INSERT ON DUPLICATE KEY. We do not intend this to be too general and will strongly recommend to use only on tables with a primary key and no other unique keys.

Yeah, I was wrong, it's not another subset, rather it's a subset of the subset that Drupal Merge implements. Merge should have been made smart to use upsert when it can. But now that upsert is more capable you can call INSERT ON DUPLICATE KEY / ON CONFLICT when only ::key/::keys/::fields were called. Then deprecate upsert and drop it in due time. Even if someone used upsert the change is easy and painless because the same API can be kept, it's only the $query = new Upsert that needs to be changed to new Merge, the rest is the same. What's wrong with this plan? I can be wrong again, I sure was before.

But I see no reason to keep both. There has been no advantage named to keeping it. None. It was just restated that we shouldn't drop it but not why. Let me ask plain: what advantage would Drupal gain from having both that justifies the doubled maintenance and double the query API people need to learn?

Since we're at this, I think we should also change Upsert::execute() to return true or fail with an exception.

The base class has this, which is a signal of fragility:

  /**
   * Executes the UPSERT operation.
   *
   * @return int
   *   An integer indicating the number of rows affected by the operation. Do
   *   not rely on this value as a precise indication of the actual rows
   *   affected: different database engines return different values.
   */

The mysqli driver in the MR cannot return an int, see comments inline.

So changing to true would just be more solid IMO.

Tests are green on all 4 core drivers.

I am working on https://git.drupalcode.org/project/drupal/-/merge_requests/14356 mostly to demonstrate what I meant: certain merge queries can be run as upserts, this doesn't need a separate upsert API.

> It's pretty obvious that it should work :)

Then why are we keeping upserts? There is still no answer to the why two APIs would be worth it. When I asked above I got an LLM generated table of what others are doing.

It seems we have a breakdown in communication. Perhaps I was not clear in my proposal:

1. replace all $connection->upsert calls with $connection->merge
2. fold all the functionality of upsert into merge.

The advantages are:

1. There's one API instead of two so developers need to learn only one.
2. There's less core code to maintain, the base upsert class can be removed and eventually, as full ANSI SQL:03 support spreads, proprietary upsert implementations can be removed as well. The first one is PostgreSQL which had ON CONFLICT DO UPDATE and now it's not necessary. Hopefully one day the entire "can we do an upsert" logic will be removed from Merge.

Can you point out any specific weaknesses of this plan? Is there any upsert usage that is not covered like this? What code snippet could you provide that justifies the existence of upsert?

#39 I would rather say one way communication only.

1) Let me be bluntly clear: I am not interested in debating this, sorry. Both @amatescu and myself already gave our input. I do not have other.

2) This issue title is 'Add support for unique / primary key constraints composed of multiple fields for Upsert queries'. You are deranging the issue. Please open another one where a MERGE vs UPSERT policy decision can be debated by those interested.

we should also change Upsert::execute() to return true or fail with an exception.

What if the query updated zero records. Will it be considered as failure?

#16

Can we use the upsert also for Drupal\Core\KeyValueStore\DatabaseStorage::setIfNotExists()?

Nope, because that method relies on the return status of the merge query, and upsert doesn't support that.

Actually IMHO we do not need MERGE nor UPSERT there: what is required is simply an INSERT, capturing with a no-op the exception that is triggered if the record exists already based on the PK. One SQL statement executed only vs the two that MERGE currently requires in Drupal.

That'd be a separate issue though.

#41

What if the query updated zero records. Will it be considered as failure?

That's an impossible situation. Assuming a single row is UPSERTED, either it does not exists yet, and then it is added (=INSERT), or it exists already, and then it is UPDATEd. So one row is always involved. A failure is due either to context/syntax (missing table, wrong key, wrong columns) or to database in invalid state (connection down, database space or quota issue, tech problems, etc). But those conditions trigger execution exceptions, should not be treated as an executed SQL statement.

Same if multiple rows are UPSERTED, with the additional complexity that the rows count for the UPDATEd rows needs to distinguish between rows matched (= the number of rows that are potentially subject to change based on the conditions) and rows changed (= the number of matched rows that had changes to column values not part of the key columns).

So IMHO returning a value from Upsert::execute() is, simply, irrelevant. We should just catch exceptions due to the factors above. It would even be better to give it a void return type.

Now, unfortunately myqsli::$affected_rows returns 0 in case UPSERT updates the row. Hence the comment in its execute() method DOCblock:

   * @return true
   *   Differently from PDO MySQL, we are not able to return a number of rows
   *   here, because $stmt->rowCount() will return a positive number only in
   *   case the UPSERT actually inserts a record (updates are not tracked).
   *   mysqli->info also does not help as the INSERT INTO ... ON DUPLICATE KEY
   *   UPDATE statement execution does not fill in any data. So we return TRUE
   *   unless an exception is thrown.

This makes mysqli inconsistent vs the other drivers, which is just a complication we are adding on our shoulders since we should not care at all about the returned value.

Let's revert the mysqli changes then, and see if tests still fail and why.

mysqli kernel tests fail after the revert, where all other drivers are green.

I think the MR here should not try to change everything at once. Instead let's just concentrate on upsert taking multiple keys (as per the issue title) and defer the conversion to followups.

I also think that key() being allowed to take string|array is the simpler API name? For the array variant, you are still passing a single composite key, structured as an array, so the name makes sense?

MR!15248 extracts the minimum set of changes from MR!12959, and keeps backward compatibility by retaining the old key() method.

This is a problem for downstream database drivers, but won't this be a problem whatever we do here? As soon as we commit a multiple key upsert to core they won't know how to handle it.

#50 maybe we could deprecate passing a string to ::key(), informing that as of D12 contrib/custom drivers have to support composite key as an array of column names? Yes won’t work if the driver overrides the method, but that should be rather an edge case.

Is that worth it though? That forces downstream users of upsert() to change all existing cases. The current MR only forces database driver maintainers (of which there are probably less than 10) to make a change to support this new feature, which they are going to have to do anyway.

#52 good point. Shall we, then?

Committed and pushed 3ae3915e6fa to main and 2e8705a4bb1 to 11.x. Thanks!