Problem/Motivation
#3058853: Drupal 8.x on Postgresql 12 showed we have a new failure on Postgresql 12, we currently test against 9.1 (and 9.5 manually).
Proposed resolution
1. Add db/pgsql-12.1 environment with postgresql-contrib package with pg_tgrm extension
2. Add "testing_fake" schema to all pgsql containers.
Remaining tasks
- Manual testing/Review
Manual testing
Setup inside a drupalci vagrant VM
- Clone drupalci_environments
- Apply the patch inside of it
- cd ~/drupalci_environments/db/pgsql-12.1
- docker build . (if successful, this will return a line like "Successfully built 7d7cfc3ec4ac")
- docker tag 7d7cfc3ec4ac drupalci/pgsql-12.1 (7d7cfc3ec4ac comes from the previous line)
- Pull some other containers like drupalci/pgsql-9.5 and drupalci/pgsql-9.1
Manually Test schema and extension output
Needs drupalci/pgsql-9.1 and drupalci/pgsql-9.5 pulled first.
for IMAGE in 9.1 9.5 12.1 ; do
docker run -d drupalci/pgsql-${IMAGE} ;
sleep 20;
docker exec -ti $(docker ps -q) psql -d drupaltestbot -c 'SELECT nspname FROM pg_catalog.pg_namespace;' ;
docker exec -ti $(docker ps -q) psql -d drupaltestbot -c 'CREATE EXTENSION pg_trgm;' ;
docker rm -f $(docker ps -q) ;
done
The output for those exec commands should look like this given 9.1 (-), 9.5 (-1), and 12.1 (+)
nspname
--------------------
pg_toast
pg_temp_1
pg_toast_temp_1
pg_catalog
public
information_schema
+ drupal
-(6 rows)
-(6 rows)
+(7 rows)
and
- ERROR: could not open extension control file "/usr/share/postgresql/9.1/extension/pg_tgrm.control": No such file or directory
- ERROR: could not open extension control file "/usr/share/postgresql/9.5/extension/pg_tgrm.control": No such file or directory
+ CREATE EXTENSION
Manually test drupalci itself
- Comment out ~/testrunner/src/DrupalCI/Build/Environment/Environment::pull() from line 365 to 381 because the new image cannot be pulled from docker hub.
- Create a development.yml file. This development.yml should run some of the test groups like Database and uses the pgsql-12.1 environment. Example attached in comment #17.
- Change directory to ~/testrunner
- Watch them all fail when running ./drupalci run /path/to/development.yml because of the 12.1 issue.
Release notes snippet
drupalci now has a PostgreSQL 12.1 database environment. This environment has the pg_tgrm extension available to install. This functionality should not be relied upon as an available dependency when developing database features in Drupal core or contrib. All PostgreSQL database environments on drupalci now include a schema within the drupaltestbot database called "testing_fake" to be used for testing only.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | interdiff-3104007-18-19.txt | 748 bytes | mradcliffe |
| #19 | 3104007-19.patch | 41.97 KB | mradcliffe |
Comments
Comment #2
mradcliffeI think this goes in the drupalci_environments project.
Didn't you mention to me about some concern with adding too many environments (based on infrastructure resources), @Mixologic? Recalling my comment #9 in #3019693: Upgrade test running to Postgres 9.5 we only added 9.5 because of that concern.
I don't think we can switch to 12.1 by default because of Drupal 8 minimum requirements.
I haven't looked at what RHEL, Ubuntu, and Debian have locked in on since my comment #25 in #2846994: Increase minimum version requirement for Postgres to 10 and require the pg_trgm extension, but that should probably be narrowed down for Drupal 9. And then the correct container added and then switched to. Once Drupal 8 is EOL we can remove the 9.1 and 9.5 containers.
I also updated the issue summary with instructions based on what I did last time.
Comment #3
catchIf it's a choice between only testing with the most recent version or only testing with the oldest version, I think we should test with the most recent version.
Comment #4
MixologicWe can add more environments now, there was a time when every environment was a checkbox and people in issues would check them all. Now they have to add them one by one, so its less of an issue.
Comment #5
mradcliffeThe main changes for 12 is that the default directories for clusters _really_ want to be the MAJOR version rather than MAJOR.MINOR now. And drupalci expects MAJOR.MINOR for the data directory. With a bit of kludging, we can make it work for both in startup.sh and postgresql.conf.
However, I'm having trouble getting my local drupalci instance to work with. composer so I'm not able to test this patch.
Comment #6
daffie commentedAfter talking to @mixologic on Slack he agreed that we could add the extension
pg_trgmto the PostgreSQL database. This is to be able to testing for the issue: #2988018: [PP-1] Performance issues with path alias generated queries on PostgreSQL . He also agreed to add a non-public schema to PostgreSQL. This is to be able to testing for the issue: #1060476: Multiple issues when PostgreSQL is used with non-public schema. We agreed to only add this to a new testbot, so that there will be no change of somebody writing code that will require the extension or the non-public to be there. If they do all other testbots will fail. Both issues will be postponed on this issue.Comment #7
mradcliffeChanging status to Needs work based on #6.
Also replicate build step is the problem with how I was testing because of composer. Using COMPOSER_ROOT_VERSION=8.9.x-dev or whatever should resolve that if I can hack it into the composer command for testing.
Comment #8
mradcliffeHere's a patch that implements @daffie's requests in #6. The schema is named "drupal", which is distinct from the "public" schema, and only in the drupaltestbot database. It installs the extension, but does not create the extension. Drupal core tests can be written against these two things in #1060476: Multiple issues when PostgreSQL is used with non-public schema and #2988018: [PP-1] Performance issues with path alias generated queries on PostgreSQL respectively.
Manual Tests for schema and extension:
The output for those exec commands should look like this given 9.5 (-) and 12.1 (+)
and
Comment #9
mradcliffeI think we should consider creating the schema in both 9.1 and 9.5 images as well and recreating those images, @Mixologic.
Comment #10
daffie commented@mradcliffe: Thanks for helping.
I am not sure if this is enough. My question is has the database user enough privileges. According to the PostgreSQL documentation: "For most extensions this means superuser or database owner privileges are needed." See: https://www.postgresql.org/docs/9.1/sql-createextension.html.
The problem that @mixologic is worried that somebody will write code that will depend on the drupal schema being there.
Personaly I am not sure what is the better thing to do in this case. I like to have the drupal schema in all PostgreSQL testbots, but @mixologic has a good point.
Comment #11
mradcliffeThe drupaltestbot user is the one that owns the database
The drupaltestbot user is the one that owns the cluster and database so
CREATE EXTENSIONwill succeed.See the manual testing steps.
Maybe the extra schema should be called "do_not_use" or "testing_fake" instead of "drupal"?
I was worried about calling it drupal too.
Comment #12
daffie commented@mradcliffe: For both points: Great!
What does now needs to happen? And who do we need for that?
Comment #13
mradcliffeI think I need to fix the patch to not call the schema "drupal" per your point, @daffie. Setting to Needs work.
Steps after something is reviewed. There's no tests here, but the patch gets merged into the dev branch and then into the production branch. @Mixologic runs one of the scripts (or a jenkins job) on the d.o infrastructure, which builds the images and publishes them to docker hub.
Comment #14
mradcliffeQuick patch fix. Back to needs review. I added my manual testing instructions in case anybody else wants to run drupalci_testbot locally.
Comment #15
daffie commentedComment #16
daffie commentedIt is my first time reviewing a DrupalCI environments issue. Please be patient with me.
The part with manual testing the drupalci_environments part works as described in the IS.
The part with manual testing in the druplci_testbot does not work for me.
I get the following error when running the drupalci command:
I have commented out the lines in ~/testrunner/src/DrupalCI/Build/Environment/Environment::pull().
I do not know what to do with the following line:
By the way, running the drupalci command with other databases/versions works fine.
I do have a couple of other questions:
Can I ask what the source is where those extra lines come from?
RUN apt-get -qq install apt-utilsor I will get the followingwarning: "debconf: delaying package configuration, since apt-utils is not installed"
Comment #17
mradcliffedrupalciwithin the VM is located insidePATH, which points to/opt/drupalci/testrunnerand not/home/testbot/testrunner. If you're using that, then it won't have the docker pull hack changes. This may also be why the test run completely failed.I do get a timeout initially, but that is expected behavior for drupalci, which will wait until it can connect to the database server before proceeding:
Good question. I went looking for this before I made the change, and I found the default for the official postgres docker image is stored at
/usr/share/postgresql/12/postgresql.conf.sample. It's also listed in the Dockerfile for that image. I then merged the manual config changes from 9.5 into that file, and then manually adjusted the # comment line endings to match so that a diff between the two would be a bit easier to read.I looked at the Dockerfile for the postgres 12/12.1 image and I didn't find apt-utils usage there. I'm not sure about the importance of apt-utils, but it probably wouldn't hurt to add it. I also got the same warning when I build the pgsql-9.5 image manually.
I agree. @Mixologic was concerned about messing with the older postgresql containers, but I think adding a schema is pretty standard since forever.
Locally? We only need to tag the built image:
docker tag IMAGE_ID drupalci/pgsql-12.1. On drupal.org? @Mixologic will run a script to build and publish new images to hub.docker.com.Thank you for the review.
Setting to needs work because I think adding the schema to pgsql-9.1 and pgsql-9.5 is a good idea too.
Comment #18
mradcliffeSomehow I changed it to Active instead of Needs work. Anyway, back to Needs review.
This patch addresses adding the schema to 9.1 and 9.5. 9.1 did not support the "IF NOT EXISTS" syntax for "CREATE SCHEMA".
After manually building pgsql-9.1 and pgsql-9.5 images,
Create a container based on each image
Use logs to see that "CREATE SCHEMA" appears after "CREATE DATABASE".
Comment #19
mradcliffeUgh. Fixed.
Comment #20
daffie commented@mradcliffe: Thanks for all your help on this issue.
After some problem, I was able to run the testbot with the new PostgreSQL 12.1 database.
All changes look good to me.
For me it is RTBC.
For @mixologic: When building the docker image I got the following error: "debconf: delaying package configuration, since apt-utils is not installed". I was able to fix it by adding the following line:
RUN apt-get -qq install apt-utils. Talked about it with @mradcliffe on Slack about it and suggested that we would you to make the decision whether or not it should be added.Comment #21
andypostThe #3058853-25: Drupal 8.x on Postgresql 12 commited, so new env could be added
Comment #22
andypostAccording to #2846994-31: Increase minimum version requirement for Postgres to 10 and require the pg_trgm extension we need 9.6 image too
Comment #23
mradcliffeShould we remove the 9.5 image that we added last year?
Comment #24
catchIt's my understanding that it's no longer an issue how many database combinations there are on DrupalCI after the ability to checkbox everything was removed - just having the containers available doesn't increase load. So I think we can independently add and remove things without having to do one in/one out. Tentatively re-RTBCing.
Comment #26
Mixologicre #23: lets not do all of the pgsql stuff in one issue. If its going to need removal, lets do that in another issue.
I'd also rather not include #18 in this change as it might have an impact on existing containers, so let's also add that to a followup.
Our automated build puts the db containers in a different place than we normally use them, so I'll have to build, and then manually repush to get this out.
Comment #27
MixologicMarking 'needs work' as I need to do things before we can mark this closed. (needs deployment?)
Comment #28
gábor hojtsy#2846994: Increase minimum version requirement for Postgres to 10 and require the pg_trgm extension proposes to raise PostgreSQL requirements to 9.6 before the beta. We don't have environments higher than PostgreSQL 9.5 now, so that patch cannot be tested currently. Therefore marking the rollout of this a beta requirement.
Comment #29
gábor hojtsyComment #30
daffie commentedCreated a followup bugfix in #3118249: Fix PostgreSQL 12 container for building after PostgreSQL 13 release.
Comment #32
MixologicOkay, I deployed this, and set up a test on 9.0.x-dev to run them: https://www.drupal.org/pift-ci-job/1604606
Comment #34
andypostFiled #3240346: Add Postgresql 15.4/16.0 to CI