Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the PgSQL driver #5880

Merged
merged 1 commit into from
Jan 31, 2023
Merged

Add the PgSQL driver #5880

merged 1 commit into from
Jan 31, 2023

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Jan 29, 2023

Q A
Type feature
Fixed issues N/A

Summary

This PR introduces a new Postgres driver that leverages PHP's pgsql extension. Postgres is currently the last database that we only support through PDO. This driver closes the gap.

ext-pgsql has a rather verbose API. Unlike other database extensions, it does not even bother to cast result types: It returns everything as a string. Except for null values: Those are really null!

However, we get information on the database type of all result fields, so we are able to perform those casts ourselves. This is why I decided to perform very basic type mappings in the driver already. Otherwise, the result sets produced by ext-pgsql would have been a permanent edge case: Booleans come as 't' and 'f', binary data is returned as hex string prefixed with 0x. Not casting those would cause a lot of functional tests to fail.

The type casts that I perform in the driver should be similar to what PDO_pgsql does internally as well. That should make a potential switch from PDO to the native extension rather seamless.

In PHP 8.1, all resource types used by the extensions have been changed to objects. This is why you will see a couple of type annotations with a union of resource and some class from the PgSql namespace. This PR currently targets DBAL 3.6 and PHP 7.4. Once this code hits the 4.x branch, we can simplify a lot of type declarations.

The one big limitation though is that I haven't implemented support for loading BLOBs from resources yet. That topic is a bit complex, so I'd like to work on that in a follow-up. For now, I think we can ship this driver without BLOB support.

@@ -19,7 +19,6 @@
<file name="vendor/jetbrains/phpstorm-stubs/ibm_db2/ibm_db2.php" />
<file name="vendor/jetbrains/phpstorm-stubs/mysqli/mysqli.php" />
<file name="vendor/jetbrains/phpstorm-stubs/oci8/oci8.php" />
<file name="vendor/jetbrains/phpstorm-stubs/pgsql/pgsql.php" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Psalm actually runs better using its own ext-pgsql stubs. 🧐

@derrabus derrabus force-pushed the feature/pgsql branch 2 times, most recently from 3703622 to 099c454 Compare January 29, 2023 14:24
src/Driver/PgSQL/Connection.php Outdated Show resolved Hide resolved
src/Driver/PgSQL/Connection.php Show resolved Hide resolved
src/Driver/PgSQL/Result.php Outdated Show resolved Hide resolved
src/Driver/PgSQL/Statement.php Outdated Show resolved Hide resolved
greg0ire
greg0ire previously approved these changes Jan 29, 2023
@derrabus
Copy link
Member Author

Running the ORM test suite uncovered a nice little bug in the type mapping logic. 😓 On it.

@derrabus
Copy link
Member Author

I've added Doctrine\DBAL\Tests\Functional\Driver\PgSQL\ResultTest::testTypeConversionWithDuplicateFieldNames() to reproduce the problem found by the ORM's test suite and fixed the issue.

The ORM's test suite passes now, but it's significantly slower with the new driver than it is with PDO. 😕

But I can investigate this in a follow-up, I guess.

src/Driver/PgSQL/Driver.php Outdated Show resolved Hide resolved
src/Driver/PgSQL/Driver.php Outdated Show resolved Hide resolved
src/Driver/PgSQL/Exception/UnknownParameter.php Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member

but it's significantly slower with the new driver than it is with PDO

Should we somehow document the new driver as experimental because of this?

@derrabus
Copy link
Member Author

Done.

@derrabus derrabus merged commit f5196ed into doctrine:3.6.x Jan 31, 2023
@derrabus derrabus deleted the feature/pgsql branch January 31, 2023 18:01
@andrew-demb
Copy link
Contributor

The ORM's test suite passes now, but it's significantly slower with the new driver than it is with PDO. 😕

It may be related to "real" prepared statements, in comparison to emulated prepared statements in PDO.

@derrabus
Copy link
Member Author

You mean because those real prepared statements mean an extra round-trip to the Posgres server? But that is nevertheless desirable, isn't it? I'm not sure I want to resolve prepared statements in the driver. 🙈

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants