Add an insertMany for the Connection class

[BackEndTea]

Q	A
Type	feature
Fixed issues	#2981

Summary

It's quite a common use case to have to insert many values into the database at once.
With the current API of the Connection class you can either: insert them 1 by 1, or insert batches by wrapping them inside a transaction.

This PR introduces the insertMany method, which inserts multiple rows at once with the INSERT INTO <table> (<columns>) VALUES (<row1>), (row2)... syntax.

What this method doesn't do

This method is not responsible for splitting the inserts into multiples when they become to big, instead the user should use something like array_chunk to decide how many inserts should be done at once:

$chunked = array_chunk($values, 1000);
foreach($chunked as $rows) {
    $this->connection->insertMany('my_table', $rows, $types);
}

There is no validation on whether the keys for the rows are the same for each entry. This method takes the first insert, and makes that the source of truth. They keys don't need to be in exactly the same order, as that would probably be the cause of hard to bebug issues.

Notes for reviewers
I'm not too happy with the logic for the types, but i'm not sure how to improve it. I'm open to suggestions

[BackEndTea]

@derrabus I have added a functional test to the WriteTest. Are there any other tests that should be added?

[derrabus]

@derrabus I have added a functional test to the WriteTest. Are there any other tests that should be added?

That's good. My main concern was that your mock fest won't ever hit a database which is a no-go for a new feature. I'll conduct a more thorough review, but at first glance it looks like you're only covering the happy path of your feature and your implementation does little input validation.

[derrabus]

Thank you for your PR.

If I remember correctly, the INSERT syntax with multiple rows is a non-standard. But I might be mistaken. TIL that all databases that I've checked in my quick research actually support it, but I don't know since when.

/edit: Okay, apparently Oracle does not support that syntax. We'll need a solution for that as well.

This makes it even more important to cover this feature with a functional test. Your first iteration only contained a unit test on a mocked connection which has very limited value: We verify if the emitted syntax is what we expect without validating if an actual database would accept it.

What's clearly missing is coverage besides the happy path:

• What if my $data array contains $row arrays of different shapes?
• What if my first row is empty?
• What if one of the rows isn't an array at all?
• What if my list of types is larger than the number of columns of the first row?

And that's the pure technical part.

We have to decide whether we want to include this feature at all. In general, we're relucant when it comes to adding more functionality to the wrapper layer – especially when the feature could very well be built outside of it. In this case, I personally do see the benefit though, but maybe the others have a different opinion.

[BackEndTea]

Okay, apparently Oracle does not support that syntax. We'll need a solution for that as well.

Would it be okay to add this information to the driver classes? And then check against that?

Also, is there a way to easily test against different databases locally? From what i'm reading online oracle should have support for this syntax, so i'd like to expermiment with it a bit.

What if my $data array contains $row arrays of different shapes?

This now throws an exception.

What if my first row is empty?

I've updated it to now insert empty values instead.

What if one of the rows isn't an array at all?

The type defined is array<array<string, mixed>>. I don't think its the responsibility of doctrine to check for that. In the same way, if the $types array contains an invalid type, it will throw a TypeError somewhere down the line.

What if my list of types is larger than the number of columns of the first row?

Those will be ignored, just like the insert, update and delete methods.

We have to decide whether we want to include this feature at all. In general, we're relucant when it comes to adding more functionality to the wrapper layer – especially when the feature could very well be built outside of it. In this case, I personally do see the benefit though, but maybe the others have a different opinion.

I fully understand if you don't want to have this in doctrine itself, in that case i'll add this to our own code base instead.

[derrabus]

Just a wild idea that came to my mind. If the user calls…

$connection->insertMany('some_table', [
    ['foo' => 1, 'bar' => 4711],
    ['foo' => 2],
]);

… my assumption would be that they's expect us to do this instead of an exception.

INSERT INTO some_table (foo, bar) VALUES
    (1, 4711),
    (2, DEFAULT)

WDYT? Of course that means that with the current implementation the first row has to specify all columns that we're about to use. Then again, maybe we should move away from giving the first row any special treatment. We could make it mandatory to specify the types array and derive the columns from there.

[derrabus]

Also, is there a way to easily test against different databases locally?

In our CI, every database is just a docker container. You can spin up any of those containers locally. If you use the same settings as we do, you should be able to use the PHPUnit configurations in ci/github/phpunit to run the tests against a specific database.

[morozov]

This is impossible (confused with upserts) It will require some more effort to implement correctly for all the dimensions (platforms, drivers, column and binding types) that the DBAL supports, which is the reason why it hasn't been implemented so far.

Relevant historical references:

1. Query string size, bound-parameter count, rows per statement: Bulk inserts #2762 (comment)
2. Named and positional parameters: Bulk inserts #2762 (comment)
3. Support across platforms: Bulk inserts #2762 (comment)
4. Another earlier attempt: [WIP] [DBAL-218] Add bulk insert query #682

[BackEndTea]

Thanks for the reviews so far. I have a few questions:

1. What are the "Query string size, bound-parameter count, rows per statement" maxes per platform? I've found it difficult to find this data other than stack overflow answers etc.
2. How do we want to deal with different syntaxes? e.g. Oracle below v23 has a different syntax for bulk inserts. For now i've opted to simply not support this. Is this okay? Or do we need to have support for different syntaxes?

@derrabus

We could make it mandatory to specify the types array and derive the columns from there.

I don't think making all types mandatory is a good idea, that is very much different from any other part of the dbal api, where we default to string types.

Then again, maybe we should move away from giving the first row any special treatment

I'm not sure what the best solution is. Requiring all rows to have the same structure would make the workings of this method a lot easier to build and maintain. For now i feel like we could first implement it where we require all rows to be the same. And if someone has a use case to allow different structures they can open a PR to add that feature.

@morozov

Thanks for the additional information/earlier attempts, that helps a lot.

[BackEndTea]

Running the tests against oracele with the oci8 extension runs into the following problem. This error does not occur for the pdo_oci extension. My guess is that some variable isn't set up correctly, but i know too little about oracle to fix this.

These tests are skipped for the pdo_oracle, as it does not support breaking the connection.

src/Driver/OCI8/Connection.php:41
 oci_server_version(): ORA-12162: TNS:net service name is incorrectly specified
| Help: https://docs.oracle.com/error-help/db/ora-12162/

There is also a lot of PHPStan issues, which as far as i can tell, are not caused by this PR.

[BackEndTea]

This causes the getServerVersion for oracle to be used in CI, which is the current failure for the oci8 extension

[BackEndTea]

I have added these sources to the doc blocks. Do we want to keep these or should i remove them?

[BackEndTea]

Should we create a new SQLite platform version which returns 32766 for post 3.32.0?, or read the SQLITE_MAX_VARIABLE_NUMBER?

[morozov]

My personal take is that using platform subclasses to represent server versions is an established anti-pattern. It doesn't mean you must not do that, it means that you don't have to. Two reasons below.

An expression like MySQL57Platform extends MySQL56Platform (which is what we usually do) implies that "MySQL 5.7 is-a MySQL 5.6", which is obviously not true. A newer version may have all the features of the older one and some new, but it also may have some breaking changes which breaks the is-a relationship.

The platform class is a god object. If platform-specific logic depends on the version, there are two better ways:

1. If the difference in logic is simple, write an if statement and branch on the version.
2. Otherwise, introduce a new API, implement it for the diverging versions and then again branch based on the version.

This vision belongs to a higher-level document and is subject for discussion, but for the lack of a better place, I'll put it here for now.

[morozov]

or read the SQLITE_MAX_VARIABLE_NUMBER

If it's readable, it's better to read and cache it in memory. A development version of PostgreSQLMetadataProvider used to do that with some Postgres-specific parameter and cached it as an object property. It's trickier with the bulk insert because there's no object that represents a Postgres connection (the driver connection is general-purpose).

[morozov]

This method is not responsible for splitting the inserts into multiples when they become to big, instead the user should use something like array_chunk to decide how many inserts should be done at once

IMO this undermines the purpose of the database abstraction: the max length of an SQL statement and the number of bound parameters depend on the target database platform, and that's exactly what this API is meant to abstract out.

What are the "Query string size, bound-parameter count, rows per statement" maxes per platform?

Are you asking about the values or meaning? The "bound-parameter count" is your getMaximumAmountOfBoundParameters(). The "query string size" depends on the number of rows passed to the method, because the SQL is built dynamically based on the input, but it cannot grow indefinitely. The DBAL, based on the platform characteristics, should limit and chunk the input into batches. As for "rows per statement", I don't know if this limitation is real, and it's the least concerning, but it's worth exploring.

[BackEndTea]

IMO this undermines the purpose of the database abstraction: the max length of an SQL statement and the number of bound parameters depend on the target database platform, and that's exactly what this API is meant to abstract out.

So far my goal has been to create a minimal version of bulk inserts that covers 90% of all edge cases. Where if we hit an unspported use case we error out.
This would allow this feature to land soon(ish), and then see what users actually need/use. Adding more features/better compatability could land in newer versions without breaking BC.

But from reading your statement it feels like you would rather this feature take a bit longer but cover more edge cases. Am i correct in that assumption?