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

x/pkgsite: test databases don't override LC_COLLATE and LC_CTYPE #40347

Closed
akolar opened this issue Jul 22, 2020 · 3 comments
Closed

x/pkgsite: test databases don't override LC_COLLATE and LC_CTYPE #40347

akolar opened this issue Jul 22, 2020 · 3 comments

Comments

@akolar
Copy link

@akolar akolar commented Jul 22, 2020

What is the URL of the page with the issue?

n/a

What is your user agent?

n/a

Screenshot

n/a

What did you do?

Run ./all.bash while using a development PostgreSQL database.

Minimal example below:

docker run --rm -d \
    -P -p 127.0.0.1:5432:5432 \
    -e POSTGRES_PASSWORD="$GO_DISCOVERY_DATABASE_TEST_PASSWORD" \
    -e LANG=en_US.UTF-8  \
   --name pg postgres:latest && \
go clean -testcache && \
./all.bash

Note that LANG=en_US.UTF-8 which means that template1 will use en_US.UTF-8 for its collation since initdb uses whatever is set in $LANG as a default value (and for non-production systems, it is reasonable to assume that $LANG is not C).

Unlike discovery-db created in https://github.com/golang/pkgsite/blob/master/devtools/create_local_db.sh, dbtest.CreateDBIfNotExists() does not override template/locale used by the CREATE DATABASE statement. This means that the two are carried over from template1 (which may or may not use C locale).

Listing all databases returns the following:

postgres=# \l
                                          List of databases
            Name            |  Owner   | Encoding |   Collate   |    Ctype    |   Access privileges   
----------------------------+----------+----------+-------------+-------------+-----------------------
 discovery-db               | postgres | UTF8     | C           | C           | 
 discovery_frontend_test    | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | 
 discovery_integration_test | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | 
 discovery_postgres_test    | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | 
 discovery_worker_test      | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | 
 postgres                   | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | 
 template0                  | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | =c/postgres          +
                            |          |          |             |             | postgres=CTc/postgres
 template1                  | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | =c/postgres          +
                            |          |          |             |             | postgres=CTc/postgres
(8 rows)

(discovery_*_test databases have collate and ctype en_US.UTF-8).

The behaviour described above is extremely flakey (for example, when the example is run on postgres:alpine instead of postgres:latest, tests pass). Related: https://www.postgresql.org/message-id/flat/23053.1337036410%40sss.pgh.pa.us#23053.1337036410@sss.pgh.pa.us

I think that the documentation should either make it clear that template1 must use C as its locale or that we should fix dbtest.CreateDBIfNotExists() and postgres.recreateDB() so that they create databases the same way ./devtools/create_local_db.sh does.

What did you expect to see?

All tests pass.

What did you see instead?

TestPostgres_GetTaggedAndPseudoVersions/want_releases_and_prereleases_only failed because entries were out of order.


Unless you have other plans with this, I'm more than happy to create a CL since most files that need changing are already identified above.

@toothrot
Copy link
Contributor

@toothrot toothrot commented Jul 22, 2020

/cc @julieqiu

@julieqiu
Copy link
Contributor

@julieqiu julieqiu commented Jul 23, 2020

Thanks, @akolar! Fixing dbtest.CreateDBIfNotExists() and postgres.recreateDB() makes sense to me - feel free to send a CL.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 23, 2020

Change https://golang.org/cl/244517 mentions this issue: internal: set collation and ctype on test databases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants