-
Notifications
You must be signed in to change notification settings - Fork 262
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
Use min_size and max_size in postgres backends #129
Conversation
databases/backends/postgres.py
Outdated
@@ -62,7 +63,9 @@ def _get_connection_kwargs(self) -> dict: | |||
async def connect(self) -> None: | |||
assert self._pool is None, "DatabaseBackend is already running" | |||
kwargs = self._get_connection_kwargs() | |||
self._pool = await asyncpg.create_pool(str(self._database_url), **kwargs) | |||
self._pool = await asyncpg.create_pool( | |||
str(urlparse(self._database_url._url)._replace(query="").geturl()), **kwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably something like this instead:
url = str(self._database_url.replace(query=''))
self._pool = await asyncpg.create_pool(url, **kwargs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomchristie @euri10 sorry for jumping into, but it'll break other things. The matter is that the query is the only way to pass other params to asyncpg
like socket
or host
that breaks the dsn parsing or caching params, etc.
self._get_connection_kwargs()
will return only pool size details and the flag for ssl
, but the rest will erased.
I think we should catch all query params in self._get_connection_kwargs()
or use an approach used in other backends -
async def connect(self) -> None:
assert self._pool is None, "DatabaseBackend is already running"
kwargs = self._get_connection_kwargs()
self._pool = await aiomysql.create_pool(
host=self._database_url.hostname,
port=self._database_url.port or 3306,
user=self._database_url.username or getpass.getuser(),
password=self._database_url.password,
db=self._database_url.database,
autocommit=True,
**kwargs,
)
The second one seems as a better option as its aligned with the unified approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that look better @gvbgduh ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it definitely does @euri10!
But what I missed is that params from query are coming through either.
>>> from databases import DatabaseURL
>>> url = DatabaseURL('pg://user:pass@/db?host=host&port=5432')
>>> url.password
'pass'
>>> url.username
'user'
>>> url.database
'db'
>>> url.port
>>> url.hostname
>>> url.options
{'host': 'host', 'port': '5432'}
_get_connection_kwargs()
accesses url.options
, but looks for specific params, and DatabaseURL
doesn't look in query for missing things.
It might be reasonable to tweak DatabaseURL
to look into options/query if it can't find params in the main url, at least for {username, password, database, port, hostname}.
If @tomchristie has no objections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's true that _get_connection_kwargs
currently removes anything that is not min_size, max_size or ssl
with an dsn like f"{POSTGRES_URLS[0]}?min_size=1&max_size=2&sslmode=require"
for instance the sslmode is lost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, it should pass it as of line https://github.com/encode/databases/blob/master/databases/backends/postgres.py#L58, but those options come from the Database
instantiation - https://github.com/encode/databases/blob/master/databases/core.py#L32.
So it'll broadcast provided kwargs as is to the DB driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see the kwarg sslmode
for the connection or for the pool in asyncpg
, so you should probably use:
ssl – Pass True or an ssl.SSLContext instance to require an SSL connection. If True, a default SSL context returned by ssl.create_default_context() will be used.
but in general you should be able to provide it as
database = Database('pg://user:pass@/db?host=host&port=5432', server_settings={...}, ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I see, thanks!
|
||
@pytest.mark.parametrize("database_url, expectation", urls_with_options) | ||
@async_adapter | ||
async def test_postgres_pool_size_connect(database_url, expectation): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could do with structuring this a bit more simply. Not sure exactly how, but I'd rather have something a bit more plain.
The empty supress
is used in a non-obvious way, and POSTGRES_URLS
is defined at a long distance from the place it's used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved POSTGRES_URLS where it's used.
as for supress we can maybe use a more explicit does_not_raise() that is either its own contextmanager (c6ab59e)
either directly an alias for ExitStack (3fd7fba)
it depends what version of python need to be supported (http://doc.pytest.org/en/latest/example/parametrize.html#parametrizing-conditional-raising)
Pool creation uses explicit url with removed query params
@gvbgduh want to take a review of where this is at now? |
@tomchristie yeah, sure, I'm actually happy with it, but we might need to clarify in docs that there's ability to provide connections params as kwargs at the UPDATE: I'm sorry, it's already in docs. |
@gvbgduh - I'll leave this with you |
I believe @euri10 was able to get it working with connection kwargs passed to |
but i'll tripple check with the fresh head tomorrow. |
I was indeed @gvbgduh
this is an alod gist but I think it's still working
https://gist.github.com/euri10/5aadc0c8f83ea81cb760c5c88c401fd9
…On Mon, Sep 30, 2019 at 2:10 PM George Bogodukhov ***@***.***> wrote:
but i'll tripple check with the fresh head tomorrow.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#129?email_source=notifications&email_token=AAINSPRCQQURFUPTLDF3XH3QMHUBRA5CNFSM4IGAXRT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD75NNSI#issuecomment-536532681>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAINSPVV7U3YSWOAFJCZCFLQMHUBRANCNFSM4IGAXRTQ>
.
--
benoit barthelet
http://pgp.mit.edu/pks/lookup?op=get&search=0xF150E01A72F6D2EE
|
Closing this as stale, to try to help us get a good handle on what |
Overtakes encode#129 Closes encode#78
Overtakes encode#129 Closes encode#78
Overtakes encode#129 Closes encode#78
Overtakes encode#129 Closes encode#78
this solves #78 not sure if it's ideal, I begun asking for comments in the issue but since anyway I had the PR made it's probably better discussing it here