Increase minimum version requirement for Postgres to 10 and require the pg_trgm extension

Which 9.2.x patch version is safe to recommend as minimal?

Also it may need to check drupal-ci images

This still needs newer pg version on ci

Guess we should jump to version 9.5., specially to finally have native "UPSERT" testing in core.

@bzrudi71 what's about 9.5 support for main LTS distros?

@andypost after some research I think we have to stuck with 9.2.x. At least RHEL 6 comes with 9.2 and has EOL Nov. 2020. Let's not force users to upgrade without a really good reason. Anyway, I still wish to have more recent PG drupalCI testing ;-)

Is there a ticket regarding upgrading drupalCI testing?

@Liam it needs to be filed to move forward

Created #3019693: Upgrade test running to Postgres 9.5.

Everything is always outdated in RHEL, that hasn't stopped us before (from bumping PHP requirements, for example).

Still, it would be good to do some research regarding who ships and supports what.

Quick search shows that Ubuntu 16.04 had 9.5 and 18.04 has 10.
Debian 8 (Jessie) has 9.4 and Debian 9 (Buster) has 9.6.
9.4 is the lowest currently supported version by upstream, supported until February 2020.

We could always announce a year in advance, like we did for PHP, then bump to 9.5 for Drupal 9.

That sound reasonable to me.

Bumping the pgsql requirement to 9.5 is a very good idea, it will allow us to use the new JSON capabilities :)

I updated the drupalci environments patch to add 9.5, but in retrospect, I think that @bojanz is right and that we'd probably have a 1-year announce because by 2022 RHEL, Debian and Ubuntu LTS will be 10 or higher.

Just opened #3104007: Postgresql 12 DrupalCI environment needed for core testing, adding as a related issue.

It's probably too late to commit this patch to Drupal 8 now, but I'm adding it as a should-have to #3079680: [META] Requirements for tagging Drupal 9.0.0-beta1.

Also we should probably increase it further for Drupal 9 to whatever the minimum supported version will be when 9.0.0 is released (or at the end of 2020?)

Also if postgresql has a predictable release schedule and support cycle, we should open a policy issue similar to #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4 and document when we'd drop support for EOL postgres versions in the future.

Created #3106077: [policy] Decide on PostgreSQL 9.x/10.x support status, @catch.

It's probably too late to commit this patch to Drupal 8 now

Agreed. At least not all the way to 9.5. I could maybe see an argument for raising it to 9.2 in Drupal 8 still, but not sure if that's worth it. If it is worth it, let's open a separate issue for that. I don't think we can go higher than 9.2 in Drupal 8, because that's the version in CentOS 7, and CloudLinux and CPanel still don't have stable releases out for CentOS 8, so I think we should assume that there's at least some Drupal 8 sites still on PostgreSQL 9.2.

Therefore, setting the version of this issue to 9.0.x. For Drupal 9, per #3106077-11: [policy] Decide on PostgreSQL 9.x/10.x support status, I propose we go to 9.6 at a minimum. Yes, that means Ubuntu 16.04 users will have to either upgrade their OS or install a newer postgresql package, but I think that's an acceptable requirement for upgrading to a new major Drupal version. Note there's additional discussion in #3106077: [policy] Decide on PostgreSQL 9.x/10.x support status about going to 10, but I think that needs more discussion before doing it, and I don't think we should hold up this issue on that.

Updating the issue title accordingly, but if anyone wants to argue for not going to 9.6 yet, feel free to do so, and we can retitle this back to 9.5.x and discuss 9.6 in a followup. Thanks!

Updated the issue summary, +1 from me to increasing to 9.6 for now asap.

Reminds me of #2549323: Improve the Upsert query implementation for PostgreSQL where we added support for UPSERT queries on PostgreSQL < 9.5. That workaround will be no longer required and all the code should be removed, so adding as related.

Looks like there is agreement on 9.6 for now. Rerolled for that. Also I found an install task check that oddly checked if PostreSQL is even version 9, which it must be given that the main version check runs before this one. May be overkill to fix it here, we can remove that section of the patch.

Also removing Needs release manager review, @catch added it and then he provided feedback a few weeks later.

Currently PostgreSQL 9.5 is the highest version available for testing. This is the list:

#3104007: Postgresql 12 DrupalCI environment needed for core testing was committed but not yet deployed. It would provide an environment to tell apart this issue failing (in all other environments) vs. the PostgreSQL 12 one.

Hm, apparently failed to apply. I had a weird checkout. Rerolled for 9 head.

WRT DB environments, we should have an environment for the minimum version core supports; otherwise, we end up with patches that rely on DB API features core does not support yet and no way of knowing from the patch. So using only PostgreSQL 12 seems overly ambitious. I think we will need 9.6 and 12, or 10 and 12, depending on whether we increase the requirement to 10 or not.

@daffie: please keep in mind that a PostgreSQL 12 container was already committed (but not yet built/deployed) for DrupalCI, see #3104007: Postgresql 12 DrupalCI environment needed for core testing. That would be a good starting point to adjust based on the decided version here.

#3079680-70: [META] Requirements for tagging Drupal 9.0.0-beta1 says:

PostgreSQL policy + implementation should be beta-deadline - i.e. either we raise the requirement before then or we're stuck with what we've got.

Given that, I recommend that we do this in two steps.
Step 1) Increase to 9.5. Because we're not blocked on DrupalCI for that.
Step 2) Once #3104007: Postgresql 12 DrupalCI environment needed for core testing (or any other container >9.5) is done, then raise the requirement to 9.6 or 10, depending on what's decided about 9.6 in #3106077: [policy] Decide on PostgreSQL 9.x/10.x support status.

This way, hopefully step 2 is done before beta, but if not, then at the very minimum we ensure that step 1 is.

Not sure whether we want to use this issue for step 1 and open a new one for step 2, or vice versa.

#41 makes sense to me.

@catch/@effulgentsia: my understanding was that @xjm did not like the prospect of Drupal being tested on PostgreSQL 12 if its minimum requirement is PostgreSQL 9.6 as that means we may still introduce code incompatible with PostgreSQL 9.6 then without noticing. So #3104007: Postgresql 12 DrupalCI environment needed for core testing would be unsuitable if that is the case.

We'd still need a postgres 12 environment, we'd just also need one for the lower version too.

@daffie so I think we should be targeting what effulgentsia says in #3106077-34: [policy] Decide on PostgreSQL 9.x/10.x support status

For the beta, set core's minimum to 10. And also require pg_trgm.
Create a contrib driver that works on 9.6 (and maybe without pg_trgm?).
In the best case scenario (the contrib driver exists and is running tests on DrupalCI, and we don't get a ton of issues from users who are still stuck on 9.6 or without access to pg_trgm), release 9.0-rc1 the same way.
In the worst case scenario (the contrib driver isn't viable before RC for whatever reason), relax core's requirements as needed for 9.0-rc1.

That means postgres 10 and with the pg_trgm extension for the DrupalCI environment.

Agreed with #46 and retitling accordingly.

#3118243: Postgresql 10 DrupalCI environment needed for core testing has been deployed, so this is unblocked. However, in https://www.drupal.org/pift-ci-job/1604603 there was a failure on the new environment so we might have a critical blocker to fix HEAD there.

There's also many failures on Postgres 12 but those can be separate, non-blocking criticals: https://www.drupal.org/pift-ci-job/1604606

Opened #3118454: Drupal\KernelTests\Core\Database\SelectTest fails on postgres 10.

Re-roll for 10.0 and added constant as we have for mysql, useful to simplify to check tests bypass

Filed #3118591: Datetime-related test failures on PostgreSQL 12 for the remaining Postgres 12 issues after #3118454: Drupal\KernelTests\Core\Database\SelectTest fails on postgres 10.

#3118454: Drupal\KernelTests\Core\Database\SelectTest fails on postgres 10 has been committed to 9.0.x, so this is unblocked now. Requeuing the Postgres 10 tests...

Looks like this needs a reroll, probably for the changes from the other DB issues.

Rerolled patch #53.

That looks like an existing random test failure: #2982755: Random failure in SchemaTest::testSchemaChangePrimaryKey with order of composite primary key. Queued a re-test.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -133,42 +142,38 @@ protected function checkEncoding() {
+        $this->fail(t("The %setting setting is currently set to '%current_value', but ...

and if we tough this line t() should be replaced

btw Checking release notes of 12 version https://www.postgresql.org/docs/12/release-12.html#id-1.11.6.7.3

There's big changes in collation handling and sorting in general (case insensitivity out of box)
As I see it needs better tuning of used collation, also I expect ICU library related bugs in different distros

Also runners depends on installed pgsql

Current PHP 7.3 CI using

pdo_pgsql

PDO Driver for PostgreSQL => enabled
PostgreSQL(libpq) Version => 9.4.25

Current PHP 7.4 CI image

pdo_pgsql

PDO Driver for PostgreSQL => enabled
PostgreSQL(libpq) Version => 11.7 (Debian 11.7-0+deb10u1)

We can should handle Postgres-12 specific issues in separate criticals. What's blocking for beta is supporting and requiring Postgres 10 only.

Once https://www.drupal.org/pift-ci-job/1606779 is green, this should be committable.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -133,42 +142,38 @@ protected function checkEncoding() {
+        $this->fail(t("The %setting setting is currently set to '%current_value', but needs to be '%needed_value'. Change this by running the following query: < code>@query</ code>", $replacements));

I agree that it's OK to remove the t() from the updated assertion, It's a slight scope creep but fine in thie circumstance. If someone wants to do that.

https://www.drupal.org/pift-ci-job/1606779 passed, so that was the existing random test failure. We may want to keep an eye on things to see if it's more frequent with postgres 10 but it's not blocking here.

Moving back to CNR. No strong opinion on #60/#63.

Do we need to add a check for the pg_trgm extension here (looks like it might be possible to query pg_available_extensions)? Or is that chicken and egg?

@catch Maybe a followup for that?

Yes good idea. Here it is #3118730: Explicitly test for pg_trgm extention in installer.

@TravisCarden is going to review the patch here (and maybe reroll it to fix the t()).

Re: #62 libpq is the connection that php uses to *connect* to pgsql, it shouldnt matter what version it is, unless there are specific changes to that lib that are affecting us. It has no bearing on the actual version of pgsql being tested against.

Just a few nits:

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
   @@ -11,6 +11,15 @@
   +   * The contrib extension pgtrgm is supposed to be installed.
   

   I think this is supposed to be "pg_trm". See https://www.postgresql.org/docs/current/pgtrgm.html.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
   @@ -133,42 +142,38 @@ protected function checkEncoding() {
   +      $query = "ALTER DATABASE \"" . $connection_options['database'] . "\" SET bytea_output = 'escape';";
   

   This isn't terribly readable. I suggest this instead:

   $query = "ALTER DATABASE '{$connection_options['database']}' SET bytea_output = 'escape';";

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
   @@ -133,42 +142,38 @@ protected function checkEncoding() {
   +        $replacements = [
   +          '%setting' => 'bytea_output',
   +          '%current_value' => 'hex',
   +          '%needed_value' => 'escape',
   +          '@query' => $query,
   +        ];
   +        $this->fail(t("The %setting setting is currently set to '%current_value', but needs to be '%needed_value'. Change this by running the following query: <code>@query<_code>", $replacements));
   

   As others have said, if we do another patch, we may as well remove the t():

   $this->fail("The bytea_output setting is currently set to 'hex', but needs to be 'escape'. Change this by running the following query: <code>{$query}<_code>");

   NOTE: I replaced the closing code tags above with <_code> due to an apparent bug in the d.o code filter.

Rolled a fix with 1 and 2 from #70. I don't agree with removing the t(), it is present in other fail() calls in this same file. Drupal reads the .po file and maintains it in memory until it can import it into the database, so it will still translate error messages. If we want to remove t()s here, there are couple others in the file and we should be consistent.

I think @andypost is referring to #3113904: [META] Replace t() calls inside of classes rather than removing the call altogether. But I think that could be done for the whole file at once as part of that overall cleanup.

Patch looks good to me now, but kicked off various environment tests just in case.

Also queued the 9.1 "expected fail" env.

So it looks like #71 is broken somehow.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -151,7 +151,7 @@ public function checkBinaryOutput() {
-      $query = "ALTER DATABASE \"" . $connection_options['database'] . "\" SET bytea_output = 'escape';";
+      $query = "ALTER DATABASE '{$connection_options['database']}' SET bytea_output = 'escape';";

Guessing this is the issue.

I'm guessing the single quotes aren't allowed as delimeters inside the query... We could instead try:
"ALTER DATABASE \"{$connection_options['database']}\" SET bytea_output = 'escape';";

Still more readable that way if not quite as increased in readaibility. ;) Otherwise we could also go back to the concatenation, which existed in HEAD too.

Also going to retest the previous patch to make sure it really is the query syntax and not something else regressing in the meanwhile. I set the pg 10 environment to test on commit so we'll know for sure in the future.

Adjusted according to #76

Excellent!

Thanks, @daffie, link updated

#83 is reasonable, and there is no real difference between 10.0 and 10. I've changed the minimum version to just '10' on commit, we could open a follow-up to increase it to 10.16 if there's a specific reason to avoid intermediate versions.

Committed 5b44684 and pushed to 9.0.x. Thanks!

https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/14988/ is rightly failing, disabling that environment.

https://www.drupal.org/pift-ci-job/1607968 is still running, hopefully my four character on-commit change wasn't wrong :P

Oopsie, this definitely should go in the release notes.

The release note should also mention the contrib driver to support Postgres 9, if/when a prototype is available.

Also published the CR.

Let's also write out a release note for this since we also are requiring a new extension. It should link the change record.

The change record also needs to be fixed to mention the new required extension, why we added it, and how sites/hosts can turn it on.

Updated the CR (needs fleshing out, but mentions the pg_tgrm extension now) and added a release note.