PostgreSQL 10 support #2893

simPod · 2017-10-19T22:12:36Z

When using Postgres 10, DBAL throws

An exception occurred while executing 'SELECT min_value, increment_by FROM "schemaname"."table_id_seq"':

  SQLSTATE[42703]: Undefined column: 7 ERROR:  column "min_value" does not exist
  LINE 1: SELECT min_value, increment_by FROM "schemaname"."table_id_se...

This fixes it as PSQL10 reads sequences definitions from pg_sequences. https://www.postgresql.org/docs/10/static/view-pg-sequences.html

lcobucci

Can you also add new tests that covers that for PGSQL 10?

lcobucci · 2017-10-21T17:03:24Z

lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php

@@ -289,7 +289,13 @@ protected function _getPortableSequenceDefinition($sequence)
            $sequenceName = $sequence['relname'];
        }

-        $data = $this->_conn->fetchAll('SELECT min_value, increment_by FROM ' . $this->_platform->quoteIdentifier($sequenceName));
+        $version = (float) $this->_conn->getWrappedConnection()->getServerVersion();


This is breaking the test suite, can you please check that?

Also Postgres 10 yet is not supported in Travis, so there's no way to 100% guarantee the compatibility at this moment.

Majkl578 · 2017-10-22T11:51:06Z

Duplicate of #2868. Waiting for Travis, see travis-ci/travis-ci#8537.

simPod · 2017-10-25T00:38:40Z

@lcobucci It's done. I believe the test was not working properly before (correct me if I'm wrong)

simPod · 2017-10-25T08:13:02Z

There was a case when null was returned for $data and this case was ignored. https://github.com/doctrine/dbal/pull/2893/files#diff-e4bd68e20fcb55598e13e0831ba281abR63 I can add null case it needed.

Majkl578 · 2017-10-25T13:40:37Z

lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php

-        $data = $this->_conn->fetchAll('SELECT min_value, increment_by FROM ' . $this->_platform->quoteIdentifier($sequenceName));
+        $version = (float) $this->_conn->getServerVersion();
+
+        if ($version >= 10) {


I think this should rather be based on Platform check, there will sure be separate platform for Postgres 10. Anyway I think it should all be done in a single PR, not fixing random pieces randomly... :/

I agree, also exposing Connection#getServerVersion() a no-go IMO

simPod · 2017-10-25T21:22:13Z

Tried to refactor. I see this project's internals for the first time tho but IMO it's good start for PSQL10. I can either continue improving this or we can close.

PillowPillow · 2017-11-11T15:05:45Z

Any news about this PR?

iquad · 2017-11-11T16:43:02Z

@PillowPillow Travis still working to implement Postgresql 10. Until that time, we have to wait.

SCIF · 2017-11-30T08:18:22Z

Just ran functional tests on Postgres10 localy and all good. 👍

lcobucci · 2017-11-30T08:40:14Z

@SCIF it's really good to hear that let's just hope that travis adds support to it soon

Majkl578 · 2017-12-20T04:31:15Z

I got tired of infinitely waiting for Travis, so here is externally installed Postgres 10: #2946. Feel free to pick that commit and build a working compat PR on top. 😎

Majkl578 · 2017-12-20T14:59:04Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -3570,4 +3570,9 @@ public function getStringLiteralQuoteCharacter()
    {
        return "'";
    }
+
+    public function getSequenceDataSQL($sequenceName, $schemaName)


DBAL requires PHP 7.1, please add parameter and return types where applicable.

It should probably be renamed to getSequenceMetadataSQL, data is not explicit.

Majkl578 · 2017-12-20T15:00:02Z

lib/Doctrine/DBAL/Platforms/Keywords/PostgreSQL100Keywords.php

@@ -0,0 +1,37 @@
+<?php
+/*


No license headers please, we've already removed them in develop.

Majkl578 · 2017-12-20T15:00:20Z

tests/Doctrine/Tests/DBAL/Driver/AbstractPostgreSQLDriverTest.php

@@ -67,7 +67,7 @@ protected function getDatabasePlatformsForVersions()
            array('9.4', 'Doctrine\DBAL\Platforms\PostgreSQL94Platform'),
            array('9.4.0', 'Doctrine\DBAL\Platforms\PostgreSQL94Platform'),
            array('9.4.1', 'Doctrine\DBAL\Platforms\PostgreSQL94Platform'),
-            array('10', 'Doctrine\DBAL\Platforms\PostgreSQL94Platform'),
+            array('10', 'Doctrine\DBAL\Platforms\PostgreSQL100Platform'),


Please use ::class.

Majkl578 · 2017-12-20T15:01:01Z

tests/Doctrine/Tests/DBAL/Schema/PostgreSQLSchemaManagerTest.php

+    public function filtersSequencesDataProvider()
+    {
+        return [
+            ['Doctrine\DBAL\Platforms\PostgreSqlPlatform'],


Please use ::class.

Majkl578 · 2017-12-21T00:41:14Z

Here's a diff of keywords in 9.6 and 10: https://www.diffchecker.com/fJ0YkHgw
There are no new reserved keywords, but some of the existing are not reserved anymore. But I'm not a DBAL guy so I can't say whether they are supposed to be removed in PostgreSQL100Keywords or not.

simPod · 2017-12-21T15:15:26Z

They were not reserved before tho.

morozov · 2017-12-22T04:08:39Z

lib/Doctrine/DBAL/Platforms/PostgreSQL100Platform.php

+		return PostgreSQL100Keywords::class;
+	}
+
+	public function getSequenceMetadataSQL(string $rawSequenceName, string $schemaName) : string


There shouldn't be a semantic difference between this method and its parents from AbstractPlatform and PostgreSqlPlatform. Otherwise, the caller which depends on the abstract interface will not know whether it should pass raw or non-raw sequence name.

morozov · 2017-12-22T04:10:24Z

lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php

@@ -289,7 +289,7 @@ protected function _getPortableSequenceDefinition($sequence)
            $sequenceName = $sequence['relname'];
        }

-        $data = $this->_conn->fetchAll('SELECT min_value, increment_by FROM ' . $this->_platform->quoteIdentifier($sequenceName));
+        $data = $this->_conn->fetchAll($this->_platform->getSequenceMetadataSQL($sequenceName, $sequence['schemaname']));


Is it really necessary to fetchAll() and then use only $data[0]? Doing fetch() and using the row directly would make more sense to me.

There's no method fetch() in connection. In my opinion, this is ok as it fetches one row

There are more fetch*() methods though, fetchArray() is the one you need.

The data then looks like
.
I personally think the current state is more verbose as I have to reference array values by column names and not by numeric indeces.

morozov · 2017-12-22T04:34:46Z

lib/Doctrine/DBAL/Platforms/PostgreSQL100Platform.php

+	{
+		$sequenceName = $this->cleanSequenceNameFromSchemaName($rawSequenceName, $schemaName);
+
+		return 'SELECT min_value, increment_by FROM pg_sequences WHERE schemaname = '.$this->quoteStringLiteral($schemaName).' AND sequencename = ' . $this->quoteStringLiteral($sequenceName);


Given that in PostgreSQL 10, the sequence metadata is available via a view, would it make sense to rework getListSequencesSQL() instead? With this approach, the client will not have to execute the SQL from getSequenceMetadataSQL() for each sequence, and the method itself will not need to be introduced on the abstract platform level.

Not sure it's a good idea. getListSequencesSQL() lists sequences. It would need to add also increment_by and min_value columns. That would change the method's functionality. This method is called before in listSequences() already.

Look at the OraclePlatform. Its getListSequencesSQL() returns list and metadata. The only reason it doesn't do the same for PostgreSQL is that prior to version 10 there was no view to fetch the metadata from.

iquad · 2017-12-22T10:55:44Z

@simPod are you going to fix/apply or need help ? We're waiting to deploy new product to production for 2 months :P I can help if you wish.

simPod · 2017-12-22T11:35:38Z

@iquad you can already use this fork so you don't have to wait :)

iquad · 2017-12-22T11:37:50Z

@simPod yeah, but need doctrine to merge them, server tests, 30+ development machine etc. Can't handle them one by one :/

morozov · 2017-12-25T18:10:04Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+
+        $sequence1Name = 'sequence_1';
+        $sequence2Name = 'sequence_2';
+        $sequence1     = new Sequence($sequence1Name);


@simPod please make sure all sequence attributes are covered by the test, not only the name.

Can you review now please? Thanks

morozov · 2017-12-25T22:51:11Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+            $actualSequences[$sequence->getName()] = $sequence;
+        }
+
+        $isOracle      = $this->_sm->getDatabasePlatform()->getName() === OraclePlatform::NAME;


Oracle is not the only supported platform which returns upper-cased column names. DB2 is the other one but it looks DBAL doesn't support sequences in DB2. In any event, checking the platform name is not the best approach.

I can think of two solutions:

Figure out why the rest of the tests in this class don't fail on Oracle without using the portability layer. For instance, testListTableColumns() expects column names to be lower-cased, however, they are upper-cased in Oracle. Must be implemented in:

dbal/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php

Line 824 in 22844d3

$name = strtolower($column->getQuotedName($this->_platform));

Use Portability\Connection with PORTABILITY_FIX_CASE and fetch_case = CASE_LOWER when instantiating the schema manager. This is hor a real production code would deal with the case discrepancy.

testListTableColumns also converts table name to lowercase self::assertEquals('test', strtolower($columns['test']->getname()));

Can do it this way then. In

dbal/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php

Line 824 in 22844d3

$name = strtolower($column->getQuotedName($this->_platform));

it just creates an array with lowercased table names as keys.

It would be a breaking change. Doing strtolower() in a test would mean that the production code should do the same by design which it not true, it's a bug.

Let's skip the test for Oracle for now and fix the schema manager in 3.0.

morozov · 2017-12-25T22:52:14Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        $sequence1InitialValue   = 1;
+        $sequence2Name           = 'sequence_2';
+        $sequence2AllocationSize = 2;
+        $sequence2InitialValue   = 2;


It's better to use different values for different parameters to make sure they are not confused during deployment or introspection.

morozov · 2017-12-25T22:56:35Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+
+        $this->assertSame($sequence2Name, $actualSequence2->getName());
+        $this->assertEquals($sequence2AllocationSize, $actualSequence2->getAllocationSize());
+        $this->assertEquals($sequence2InitialValue, $actualSequence2->getInitialValue());


Not sure the assertions are intentionally this verbose, but the same could be checked in a more compact way:

$sequence1 = new Sequence('sequence_1', 1, 2); $sequence2 = new Sequence('sequence_2', 3, 4); // create the sequences self::assertArraySubset([ $sequence1, $sequence2, ], $this->_sm->listSequences());

I'm fan of verbosity but can remake if you think it better suits code style of this project. Should I proceed then? :)

Depending on what's the purpose of this verbosity. The approach I suggested is more state-based. It only tests that the collection returned by listSequences() contains the objects which were previously created. Isn't it what it's supposed to do?

The current approach also tests the public API of the Sequence class. For instance, if there was some logic in getInitialValue and getAllocationSize(), the test would have to take this logic into account as well which would make it less stable.

There are no strict coding guidelines in this regard, so it's up to you how you want to structure the test.

I agree. However, assertArraySubset asserts by keys and there are other Sequences created in db as well.

I reworked it a bit

It's failing on Psql 9.5. I'd have to either sort it by name or revert to the previous verbose version.

Eh… I didn't know assertArraySubset() is strict about numeric keys. The last approach could fail on any DB because the order of sequences is not enforced.

The problem is not verbosity itself, the problem is also that having the logic like array_map() or array_filter() outside of assertions which makes the test less readable and less clear about which exactly case it covers.

How about this:

$list = $sm->listSequences(); self::assertContains($sequence1, $list); self::assertContains($sequence2, $list);

That wouldn't work as those are different objects.

I guess in the end this is the simplier solution:

$this->assertSame($sequence1Name, $actualSequence1->getName()); $this->assertEquals($sequence1AllocationSize, $actualSequence1->getAllocationSize()); $this->assertEquals($sequence1InitialValue, $actualSequence1->getInitialValue());

Okay, let's leave it like this.

Kk, it's there

Majkl578

Just last few nitpicks, otherwise LGTM. 👍

lib/Doctrine/DBAL/Platforms/Keywords/PostgreSQL100Keywords.php

Majkl578 · 2017-12-26T03:15:42Z

lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php

-        $data = $this->_conn->fetchAll('SELECT min_value, increment_by FROM ' . $this->_platform->quoteIdentifier($sequenceName));
+        if ( ! isset($sequence['increment_by'], $sequence['min_value'])) {
+            $data     = $this->_conn->fetchAssoc('SELECT min_value, increment_by FROM ' . $this->_platform->quoteIdentifier($sequenceName));
+            $sequence = array_merge($sequence, $data);


Is array_merge needed here? Wouldn't + just work since it's assoc?

Majkl578 · 2017-12-26T03:17:41Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        $actualSequence1 = $actualSequences[$sequence1Name];
+        $actualSequence2 = $actualSequences[$sequence2Name];
+
+        $this->assertSame($sequence1Name, $actualSequence1->getName());


Please call all asserts statically.

Isn't that deprecated?

What do you mean? All assert methods are declared static: https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/Assert.php

Ah, not deprecated but it's there because of something old sebastianbergmann/phpunit#1914 (comment).

Both can be used tho. I'm used to call it via non-static way.

I guess it needs to be reworked to static then as the whole project uses static calls. Shall I?

And bad design in PHPUnit apparently, since non-static TestCase extends static Assert class. :/
Since it's declared as static, it was decided to use static calls in tests for assert methods, otherwise it's different context (and personally I think PHP should emit a warning, just like static->instance call).

tests/Doctrine/Tests/DBAL/Driver/AbstractPostgreSQLDriverTest.php

simPod · 2017-12-26T14:43:28Z

@morozov thanks for help!

simPod · 2017-12-26T20:51:13Z

@lcobucci is there anything else?

Ocramius

LGTM 🚢

sergiors · 2017-12-26T23:28:21Z

OMG!

iquad · 2017-12-29T08:46:24Z

thank you so much, all of you!

anonim1133 · 2018-01-02T10:26:35Z

Is there any date for 2.7 so I could use postgres 10? Or is there a way to patch 2.6 with this commit?

Ocramius · 2018-01-02T10:29:23Z

No date yet. You can already use 2.6 with PostgreSQL 10, but the schema tools won't be working correctly yet.

Use this before 2.6 branch will be patched doctrine#2893

nicholasruunu · 2019-10-01T09:57:37Z

Still have this issue with v2.9.2 and postgres 10.10

exebece · 2019-10-01T10:09:11Z

I think this issue can easily be solved using proper doctrine connection string specifying the version of postgres like: DATABASE_URL=postgres://user@127.0.0.1:5432/db_name?charset=utf8&serverVersion=10

the serverVersion part helped me solving all issues with Doctrine & Postgres. Try it for free ;)

nicholasruunu · 2019-10-01T12:33:51Z

@exebece Yeah, my bad, updated the doctrine configuration from 9.x to 10 and it works just fine now. Thanks.

XanderKon · 2020-01-18T19:51:59Z

@exebece Thanks! Spent two hours and your comment save me from another time wasting.

lcobucci suggested changes Oct 21, 2017

View reviewed changes

lcobucci added Compatibility Databases Drivers Missing Tests PostgreSQL labels Oct 21, 2017

Majkl578 reviewed Oct 25, 2017

View reviewed changes

This was referenced Nov 17, 2017

add postgresql 10 compatibility to dbal nextcloud/3rdparty#70

Merged

add postgresql 10 compatibility to dbal nextcloud/server#7210

Merged

simPod mentioned this pull request Nov 23, 2017

Doctrine\DBAL\Exception\InvalidFieldNameException when working with DB schema on PostgreSQL 10 #2868

Closed

sergiors mentioned this pull request Nov 30, 2017

it looks like this broke in psql 10 #2928

Closed

dsferruzza mentioned this pull request Dec 12, 2017

Problem upgrading from 2.2.3 to 2.3.0 with PostgreSQL 10 wallabag/wallabag#3479

Closed

Attempt to run PostgeSQL 10 on Travis

1cc2d24

Majkl578 suggested changes Dec 20, 2017

View reviewed changes

Majkl578 requested a review from morozov December 21, 2017 22:51

morozov reviewed Dec 22, 2017

View reviewed changes

morozov reviewed Dec 25, 2017

View reviewed changes

Majkl578 mentioned this pull request Dec 25, 2017

Attempt to run PostgeSQL 10 on Travis #2946

Closed

morozov approved these changes Dec 26, 2017

View reviewed changes

Majkl578 suggested changes Dec 26, 2017

View reviewed changes

Fix getting portable sequence definition for PSQL10

83b72c1

Majkl578 approved these changes Dec 26, 2017

View reviewed changes

Ocramius approved these changes Dec 26, 2017

View reviewed changes

Ocramius added this to the 2.7.0 milestone Dec 26, 2017

Ocramius merged commit fcc00de into doctrine:master Dec 26, 2017

Majkl578 removed the Missing Tests label Dec 27, 2017

morozov mentioned this pull request Jan 11, 2018

Update PostgreSqlSchemaManager.php #2978

Closed

dunglas mentioned this pull request Jan 23, 2018

Bump deps. Downgrade to Postgres 9.6. api-platform/api-platform#532

Merged

xbg pushed a commit to xbg/dbal that referenced this pull request Feb 20, 2018

Add postgresql 10 compatibility to dbal

5bebd9c

Use this before 2.6 branch will be patched doctrine#2893

xbg pushed a commit to xbg/dbal that referenced this pull request Feb 22, 2018

Add postgresql 10 compatibility to dbal

17cfa7c

Use this before 2.6 branch will be patched doctrine#2893

Majkl578 mentioned this pull request Mar 28, 2018

postgresql 10 error #3072

Closed

Kdecherf mentioned this pull request Oct 14, 2018

Error setting up database on fresh install wallabag/wallabag#3739

Closed

werrolf mentioned this pull request Jun 4, 2019

Fix #2868 on 2.5: support PostgreSQL 10 when looking up sequence metadata #3587

Closed

morozov assigned Ocramius Jun 7, 2019

github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2022

PostgreSQL 10 support #2893

PostgreSQL 10 support #2893

Conversation

simPod commented Oct 19, 2017 • edited Loading

lcobucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 commented Oct 22, 2017

simPod commented Oct 25, 2017

simPod commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simPod commented Oct 25, 2017

PillowPillow commented Nov 11, 2017

iquad commented Nov 11, 2017

SCIF commented Nov 30, 2017

lcobucci commented Nov 30, 2017

Majkl578 commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 commented Dec 21, 2017

simPod commented Dec 21, 2017

morozov Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iquad commented Dec 22, 2017

simPod commented Dec 22, 2017

iquad commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simPod Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simPod commented Dec 26, 2017

simPod commented Dec 26, 2017

Ocramius left a comment

Choose a reason for hiding this comment

sergiors commented Dec 26, 2017

iquad commented Dec 29, 2017

anonim1133 commented Jan 2, 2018

Ocramius commented Jan 2, 2018

nicholasruunu commented Oct 1, 2019

exebece commented Oct 1, 2019

nicholasruunu commented Oct 1, 2019

XanderKon commented Jan 18, 2020

simPod commented Oct 19, 2017 •

edited

Loading

morozov Dec 22, 2017 •

edited

Loading

simPod Dec 26, 2017 •

edited

Loading