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.

Sign up for GitHub

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

Jump to bottom

PostgreSQL Support #257

Merged
merged 16 commits into from
Mar 23, 2022
Merged

PostgreSQL Support #257

merged 16 commits into from
Mar 23, 2022

Conversation

mrpgraae
Copy link
Collaborator

@mrpgraae mrpgraae commented Mar 1, 2022

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.

@codecov
Copy link

codecov bot commented Mar 1, 2022 •
edited
Loading

Codecov Report

Merging #257 (f0257fc) into main (f3b3e26) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            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
Impacted Files Coverage Δ
terracotta/drivers/relational_meta_store.py 98.90% <ø> (ø)
terracotta/config.py 100.00% <100.00%> (ø)
terracotta/drivers/__init__.py 100.00% <100.00%> (ø)
terracotta/drivers/postgresql_meta_store.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3b3e26...f0257fc. Read the comment docs.

mrpgraae
mrpgraae commented Mar 1, 2022
View reviewed changes
terracotta/drivers/postgresql_meta_store.py Outdated

def _create_database(self) -> None:
engine = sqla.create_engine(
self.url.set(database='postgres'), # `.set()` returns a copy with changed parameters
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Maybe DEFAULT_CONNECT_DB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a brief explaining comment, I think that is good

mrpgraae
mrpgraae commented Mar 2, 2022
View reviewed changes
terracotta/config.py
Comment on lines +76 to +80
#: 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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to rename these to SQL_USER/SQL_PASSWORD and then deprecate the MYSQL_ fields?

Copy link
Contributor

@chapmanjacobd chapmanjacobd Mar 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be nice to support PG* environment variables

https://www.manniwood.com/2005_01_01/using_the_pg_environmental_variables_to_make_your_shell_scripts_more_terse.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #258

@mrpgraae mrpgraae marked this pull request as ready for review March 2, 2022 14:05
@mrpgraae
Copy link
Collaborator Author

mrpgraae commented Mar 2, 2022

Docs still need to be updated, but code should be ready for review 🙂

@dionhaefner
Copy link
Collaborator

I'm up to my neck with thesis work at the moment, so I'll trust your best judgment. I agree on deprecating the MYSQL_ settings, having different settings just seems silly.

@mrpgraae
Copy link
Collaborator Author

mrpgraae commented Mar 5, 2022

I'm up to my neck with thesis work at the moment, so I'll trust your best judgment. I agree on deprecating the MYSQL_ settings, having different settings just seems silly.

Cool, thanks Dion. I'll merge this PR and update docs separately. Hope the remaining thesis work goes smoothly 🙂

nickeopti
nickeopti reviewed Mar 9, 2022
View reviewed changes
Copy link
Contributor

@nickeopti nickeopti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks very nice to me!

tests/conftest.py Outdated Show resolved Hide resolved
@mrpgraae mrpgraae requested a review from nickeopti March 22, 2022 11:50
@mrpgraae
Copy link
Collaborator Author

@nickeopti I made the requested changes now. Should be good to go I think 🙂

@mrpgraae mrpgraae merged commit 343a946 into main Mar 23, 2022
@mrpgraae mrpgraae deleted the postgres-support branch March 23, 2022 13:13
@j08lue j08lue mentioned this pull request Dec 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapmanjacobd chapmanjacobd chapmanjacobd left review comments

@dionhaefner dionhaefner Awaiting requested review from dionhaefner

@nickeopti nickeopti Awaiting requested review from nickeopti

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mrpgraae @dionhaefner @chapmanjacobd @nickeopti