database_exists: don't connect to 'postgres' data base for database existence check

[bernt-matthias]

On our installation we don't have CONNECT permission for the data base called 'postgres'. Hence the function did not work.

This has been introduced here (the if else branch probably made sense in drop and create, but it doesn't here in my opinion)

5d741c1
d7f2905

[bernt-matthias]

ping @mvdbeek @erasche

[bernt-matthias]

Maybe the same should be done for create and drop data base?

[erseco]

This PR will be really helpfull when running on Heroku, because they remove CONNECT privilege in postgres database

You can replicate this behavior in your local database:

CREATE DATABASE test;
CREATE USER testuser WITH PASSWORD 'testpassword';
REVOKE CONNECT ON DATABASE postgres FROM PUBLIC;
REVOKE ALL PRIVILEGES ON DATABASE postgres from testuser;

Now you can connect using DATABASE_URL='postgres://testuser:testpassword@localhost/test' and get the CONNECT error when calling to database_exists() function.

[sveinugu]

@kvesteri A gentle nudge in your direction in order to get this pull request accepted.

I am in the exact same same situation as the issue creator @bernt-matthias, that I cannot directly deploy a production instance of the Galaxy framework for life science research (https://github.com/galaxyproject/galaxy), as our database is hosted on an university-wide service that does not provide CONNECT access to the postgres database.

There has been a need to implement workarounds downstream for a longer time (galaxyproject/galaxy#7941, usegalaxy-eu/galaxy@59305ef, CTFd/CTFd#1403 (comment)), and I would rather not have to implement another one.

It seems to me that this pull request should not be controversial.

[sveinugu]

Should also fix #330.

[CaselIT]

This change breaks database_exists if an user does not have a db called like its username.

It errors with (psycopg2.OperationalError) FATAL: database "<username-here>" does not exist

[CaselIT]

It is probably better to use one of the template database

[bernt-matthias]

Yep, I can reproduce this from the logs in Galaxy's testing framework (galaxyproject/galaxy#9967). Your suggestion is to revert the if .. else .. block but use a different value for url.database? Any suggestions?

[CaselIT]

Template0 or template1 should probably work. But it was reverted in this commit d7f2905 so they may have problems.

Probably the best solution is to add a parameter in the function, called maybe postgres_db='postgres' that can be used to specify an existing db by an user. It could probably default to postgres, and for cases where it is not available an user can change it

[bernt-matthias]

did some experiments (for postgres .. since the PR should not change anything for other DBs). The following works for me (python 3.6, sqlalchemy 1.3.17, psycopg2-binary 2.8.4):

from copy import copy
import sqlalchemy as sa
from sqlalchemy.engine.url import make_url

def ex(url):
    url = copy(make_url(url))
    database, url.database = url.database, None
    engine=sa.create_engine(url)
    result_proxy = engine.execute("SELECT 1 FROM pg_database WHERE datname='%s'" % database)
    return result_proxy.scalar()

print(ex("postgresql://USER:PASSWORD@HOST/DATABASE"))

works. So even if I see the error in the Galaxy CI logs .. I can not reproduce it.

Any idea where the information on the user may be originating from in the error case?

[CaselIT]

by default postgres connects to a db called as the user, so if a db with that name exits the check has success, otherwise it errors.

To reproduce just create a new user in the db and connect with that user.

[bernt-matthias]

Do you think that there is always a "public" data base, i.e. one with a connect privilege?

[CaselIT]

From the documentation template0 and template1 should always be defined, (unless someone has not removed them) so I think we could use template0 for the connection (since template1 is the default db used then creating a db).
But I think the best solution is to add an argument to the function and set it to postgres or template0 as default, so that it can be change in case of unusual situations.

[bernt-matthias]

Was thinking more about a good solution. How about having an additional parameter postgres_db=["template0", "template1", "postgres"] and we check all entries in the list. If one does not raise an exception its fine, otherwise the user can another value.

In this way the function can be used in a wider variety of deployments without configuration.

[CaselIT]

yes, that also works

[bernt-matthias]

Some ideas over here #463