#2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test introduced a workaround in pgsql/Schema::findPrimaryKeyColumns() due to the lack of the array_position() function in PostgreSQL < 9.5. With #2846994: Increase minimum version requirement for Postgres to 10 and require the pg_trgm extension Drupal 9 now requires PostgreSQL 10 so we can get rid of the workaround.
Use array_position() to sort the primary key in SQL instead of doing it in PHP.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3119170-2.patch | 1.4 KB | tstoeckler |
Comments
Comment #2
tstoecklerHere's a patch which is literally the reverse of the interdiff of #2881522-28: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test.
I noticed when refreshing my memory about all this over in https://www.postgresql.org/docs/10/functions-array.html#ARRAY-FUNCTIONS-... that
array_position()is documented as starting to count at 1, but I checked locally that the return value correctly starts counting 0 andSchemaTestdoes pass with this.I am not sure if this will impact #2982755: Random failure in SchemaTest::testSchemaChangePrimaryKey with order of composite primary key, but this does touch the part of the code that seems to be randomly failing.
Comment #3
daffie commentedHi @tstoeckler: THanks for working on this.
I like the new query and I do think that it would hopefully solve #2982755: Random failure in SchemaTest::testSchemaChangePrimaryKey with order of composite primary key. We can fix the whole key thing by returning it with an
array_values(). What is your idea @tstoeckler?Comment #4
tstoecklerRe #3: Sorry for being unclear above. The keys are already correctly returned as starting with 0 and SchemaTest does in fact (implicitly) test for that. I was just confused a bit because as I read the documentation I had expected it starting at 1 and wanted to clarify the situation in case anyone else was looking into it, as well. (Apparently the "clarifying" part didn't work out that well... ;-))
Comment #5
daffie commentedThank you for the explanation @tstoeckler.
The code change looks good to me.
It fixes the @todo in the method
Drupal\Core\Database\Driver\pgsql\Schema::findPrimaryKeyColumns().The query now uses the function
array_position().The order of select columns has been reversed. It was reversed because our method needed to do the ordering and that is no longer necessary.
For me it is RTBC.
Comment #6
alexpottSo this change looks great but it's going to break the fallback driver. See https://git.drupalcode.org/project/pgsql_fallback/-/blob/1.0.x/src/Drive...
Comment #8
daffie commentedThe change in the query comes from the added
array_position(). This function was added to PostgreSQL 9.5. See https://www.postgresql.org/docs/release/9.5.0/. Therefor it is not blocking the PostgreSQL fallback driver which has a minimum required version of PostgreSQL 9.6.Comment #9
alexpott@daffie thanks for confirming that. The point still still stands the fallback driver really should not be extending the core postgres classes at all. That creates a coupling that will ensure we break the fallback driver regularly and the core drivers themselves are not API.
Committed and pushed e5b0277a40 to 9.1.x and 5de150ac16 to 9.0.x. Thanks!
Backported to 9.0.x as this code is only called in tests and is currently responsible for a random fail which this hopefully fixes.
Comment #12
daffie commented@alexpott: Do you know an alternative for the way the fallback driver does it now. Then, please say so!