PostgreSQL Support #257
PostgreSQL Support #257
Conversation
Codecov Report
@@ Coverage Diff @@
## main #257 +/- ##
==========================================
+ Coverage 98.54% 98.57% +0.02%
==========================================
Files 48 49 +1
Lines 2136 2175 +39
Branches 299 304 +5
==========================================
+ Hits 2105 2144 +39
Misses 19 19
Partials 12 12
Continue to review full report at Codecov.
|
|
|
||
| def _create_database(self) -> None: | ||
| engine = sqla.create_engine( | ||
| self.url.set(database='postgres'), # `.set()` returns a copy with changed parameters |
There was a problem hiding this comment.
Postgres does not allow to connect without connecting to a specific database.
postgres is a database that is automatically created when a PostgreSQL cluster is created.
The Postgres docs say that this database "is meant as a default database for users and applications to connect to", so it should be a safe default.
There was a problem hiding this comment.
That is also what I had in mind.
Perhaps add a DEFAULT_DB_FOR_MANAGING = 'postgres' (if you can think of a better name, please use that) class variable, such that if someone somehow have managed to delete/rename their postgres database, they still have the option of creating a TC database, by overwriting that class variable
There was a problem hiding this comment.
Good idea. Maybe DEFAULT_CONNECT_DB.
There was a problem hiding this comment.
With a brief explaining comment, I think that is good
| #: PostgreSQL database username (if not given in driver path) | ||
| POSTGRESQL_USER: Optional[str] = None | ||
|
|
||
| #: PostgreSQL database password (if not given in driver path) | ||
| POSTGRESQL_PASSWORD: Optional[str] = None |
There was a problem hiding this comment.
We might want to rename these to SQL_USER/SQL_PASSWORD and then deprecate the MYSQL_ fields?
There was a problem hiding this comment.
it might be nice to support PG* environment variables
There was a problem hiding this comment.
I see I have actually broken the MYSQL_* settings with the SQLAlchemy branch :/ Seems I have somehow overlooked their existence... I'll fix that, and rename them SQL_USER/SQL_PASSWORD in the process, in a new PR shortly.
There was a problem hiding this comment.
@chapmanjacobd I have thought about this, but I think it would be best to avoid using the PG* variables. Every other env var that Terracotta reads is prefixed with TC_ so for consistency I think we should stick to that.
There was a problem hiding this comment.
@nickeopti Okay, I won't add them here then. If we rename the existing variables, it will break backwards compatibility for users who are already using MySQL. Since we are heading for a 1.0 release, maybe that's fine?
There was a problem hiding this comment.
I'll leave decisions regarding backwards compatibility and deprecation behaviour to @dionhaefner, @j08lue and/or you @mrpgraae. I don't know the use cases well enough
|
Docs still need to be updated, but code should be ready for review 🙂 |
|
I'm up to my neck with thesis work at the moment, so I'll trust your best judgment. I agree on deprecating the |
Cool, thanks Dion. I'll merge this PR and update docs separately. Hope the remaining thesis work goes smoothly 🙂 |
|
@nickeopti I made the requested changes now. Should be good to go I think 🙂 |
Adds a PostgreSQL meta store.
Would have loved to use the pure-python psycopg3, but it will unfortunately not be supported by SQLAlchemy until 2.0.