Aiopg by gvbgduh · Pull Request #124 · encode/databases

gvbgduh · 2019-07-10T16:58:43Z

No description provided.

lovelydinosaur

🤩 Let me know at whatever point you need my input on this. Looking good!

gvbgduh · 2019-07-10T17:17:09Z

Hi @tomchristie, I was finally able to finish the first turn around and there're some issues appeared.

postgresql+aiopg doesn't work for sqlalchemy as it doesn't recognise that driver (yet?) https://docs.sqlalchemy.org/en/13/core/engines.html#postgresql
so, I took the postgresql+psycopg2 one for start.
I have in mind a couple more ideas how to approach:

transform +aiopg to +psycopg in the __repr or some cast, but it doesn't feel right
pass/specify a particular backed to the Database.__init__, it might allow more flexibility later, but it's not a small architectural change and I'm not fully sure how it's aligned.

Also there might be some confusion as asyncpg is implicit choice for Postgres (even though I believe a good one :)).

Update: All in all, we just need to distinguish asyncpg and aiopg at the database class and supply a url sqlalchemy can recognise (for alembic or some other things).
Or open as issue in sqlalchemy to add an alias for aiopg, but in fact it uses psycopg2 behind the scenes utilising the async api.

A bit different handling for JSON fields values, but I need to triple check.
lastrowid seems to be not exactly the record id, but a local op id - https://aiopg.readthedocs.io/en/stable/core.html#aiopg.Cursor.lastrowid. But I'd rather triple check it as well.
A bit different approach for transactions, basically only raw queries can allow this.

In overall, it's pretty close to mysql backend, but there're some differences in details.
I also spotted a couple of places for optional refactoring - mostly moving some stuff to conftest for unittests, but there're a couple of methods that are some across all backend implementations.

Yeah, it would great to have any feedback, recommendations, complains.

lovelydinosaur · 2019-07-10T17:52:38Z

I suppose sqlalchemy should only see it as postgresql. We're not using their drivers, so that's the best information we can give it.

gvbgduh · 2019-07-12T17:17:32Z

Yes, I see, it clarifies and makes it easier.
Ok, now I would really appreciate a review if it could be possible.

mpyatishev · 2019-09-03T05:56:10Z

@tomchristie, @gvbgduh when do you plan to merge this PR?

eien · 2019-09-20T21:44:44Z

Yup, it'd be great to get this in!

lovelydinosaur · 2019-09-30T09:54:51Z

+
+
+def test_aiopg_ssl():
+    backend = AiopgBackend("postgres+aiopg://localhost/database?ssl=true")


Looks like we're expecting postgres+aiopg in some places and postgresql+aiopg in others.
I'd be okay with us accepting either or both.

Right, yeah, it's a typo, but we only expect postgresql+aiopg as of now, at least there's only this mapping. it's not triggered in these tests as don't make any use of it. I'll update urls to avoid confusion.

lovelydinosaur · 2019-09-30T10:36:17Z

+            # Apparently for `aiopg` it's OID that will always 0 in this case
+            # As it's only one action within this cursor life cycle
+            # It's recommended to use the `RETURNING` clause
+            # For obtaining the record id


This is a good example of where we ought to be returning a more structured type from .execute - that way we could have different drivers either support or not support result.row_id.

Can we change our query here to make it consistently return the pk across all our implementations maybe?

Yes, sounds reasonable indeed.
There're two ways though as I can see:

provide pure driver experience with minimal overhead, hence it'll vary from driver to driver, but will keep it as simple as possible if it can matter, or

provide unified API and implement the missing bits providing the missing functionality, but allocating the additional overhead

Oh, just 25 secs later, ok, let's follow the second path, all in all, there's always a way to jump to the raw driver and get the full manual control over it.

@tomchristie I'm thinking quite hard about it. There's a function in postgres - lastval() that can be executed to retrieve the value, but it's not reliable as there're circumstances where it can return the wrong value. The returning clause is the best way to do it, but we can't assume how the PK column is called and returning * won't be consistent with other back-ends.

lovelydinosaur · 2019-09-30T10:38:20Z

+            values = {"data": data}
+
+            if database.url.scheme == "postgresql+aiopg":
+                await database.execute(query, {"data": json.dumps(data)})


Can we handle this automatically for our users?
Would that be a sensible thing to do here?

Not sure it's possible in all cases, as I would need to know that the column of a type json, so it'll require either a lookup to the sqlalchemy's metadata or to the DB schema. But I'll have a look at how easy it can be.

it'll require either a lookup to the sqlalchemy's metadata or to the DB schema

If it means a lookup then let's not do it. I'm just not clear why JSONField isn't already handling the serialization for us in this case. Might need some more looking into?

yeah, I'm dealing with it now, there're several facets to approach it.
it can work canonically, but the field has to be sqlalchemy.dialects.postgresql.JSON, but aiopg returns back (over select) the decoded value and it breaks the RowProxy as it expects string.
so, i'm looking for compromise/settings to get the balance

ok, it was the matter of the dialect class, I reused the one that's in use for the integration with sa part of aoipg it got back to normal.

lovelydinosaur · 2019-09-30T10:49:36Z

                    await cursor.executemany(insert_query, values)
+                elif database.url.scheme == "postgresql+aiopg":
+                    cursor = await raw_connection.cursor()
+                    # No async support for `executemany`


Okay, in these kinds of cases we should still support the .executemany API, even if all it actually does is for value in values: ...execute... under the hood.

(We can document limitations of particular drivers, but it'd def. be a benefit if we can keep the API the same, even if there's different performance characteristics in some cases.)

Yeah, I see and agree, will try my best.

Ok, it's the test for the raw driver across different backends, but aiopg has the implementation for execute_many and yeah it's with the for loop.

Grand, let's just hook our backend implementation up to that then, right?

All should be in place! I have a unittest to test it for database.execute_many.

this case is the battle test of the raw driver, so it replicates backend implementations in some places.

lovelydinosaur · 2019-09-30T10:50:45Z

        database_url = DatabaseURL(url)
        if database_url.dialect == "mysql":
            url = str(database_url.replace(driver="pymysql"))
+        elif database_url.driver == "aiopg":


Let's check the .scheme here instead in these conditionals.

lovelydinosaur · 2019-09-30T10:53:16Z

Yes yes yes! Let's make this a priority. 🚀
Some review comments above - @gvbgduh feel free to keep nudging me until we've got this one in!

gvbgduh · 2019-09-30T10:54:19Z

Awesome, thanks @tomchristie!

gvbgduh · 2019-10-03T09:42:53Z

@tomchristie I think it's in good shape now. There's only one open question - lastrowid, but I'm not sure about the universal, consistent and reliable solution for that. Happy to follow up on it in the next PR if it's more reasonable.

gvbgduh · 2019-10-29T10:21:08Z

hi @tomchristie! A gentle nudge here, I think it's mergeable. Happy to follow up on any appeared issues anyway.

ghost · 2020-01-17T15:15:47Z

What would be the point of this if databases already works with asyncpg?

gvbgduh · 2020-02-05T13:59:27Z

@StephenCarboni There's a bunch of different reasons, but the main point is asyncpg and aiopg are totally different, including underlying lower-level drivers, internal and external APIs, and even query syntax details.
asyncpg is written from scratch without dependencies, while aiopg is based on psycopg2 which is based on libpq and shares the same (or very similar) API with aiomysql/aiosqllite/aioodbc.
Despite personal preferences, it can also cover different migration strategies between DBs, or from sync to async, when the time is the critical factor for example.

gvbgduh added 2 commits July 1, 2019 02:40

[WIP] aiopg support encode#39

4651671

Checks for failing unittests

66ec568

lovelydinosaur reviewed Jul 10, 2019

View reviewed changes

Sticking with postgresql+aiopg and some clean up

83c1455

gvbgduh marked this pull request as ready for review July 12, 2019 17:00

gvbgduh added 2 commits July 13, 2019 02:35

Add aiopg to the travis build

bf4dfe6

Adjustments for requirements.txt and setup.py

cefc343

gvbgduh mentioned this pull request Sep 2, 2019

Support for MS SQL #133

Open

lovelydinosaur reviewed Sep 30, 2019

View reviewed changes

Fix the dsn for some tests

7e37d41

lovelydinosaur reviewed Sep 30, 2019

View reviewed changes

gvbgduh added 2 commits October 2, 2019 22:27

Minor fixes and linter

3052211

Change the dialect for aiopg for better support.

9e58341

Merge branch 'master' into aiopg

4198ae2

gvbgduh merged commit 7417461 into encode:master Mar 16, 2020



		def test_aiopg_ssl():
		backend = AiopgBackend("postgres+aiopg://localhost/database?ssl=true")

Conversation

gvbgduh commented Jul 10, 2019

Uh oh!

lovelydinosaur left a comment

Choose a reason for hiding this comment

Uh oh!

gvbgduh commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lovelydinosaur commented Jul 10, 2019

Uh oh!

gvbgduh commented Jul 12, 2019

Uh oh!

mpyatishev commented Sep 3, 2019

Uh oh!

eien commented Sep 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvbgduh Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvbgduh Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lovelydinosaur Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lovelydinosaur commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvbgduh commented Sep 30, 2019

Uh oh!

gvbgduh commented Oct 3, 2019

Uh oh!

gvbgduh commented Oct 29, 2019

Uh oh!

ghost commented Jan 17, 2020

Uh oh!

gvbgduh commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

gvbgduh commented Jul 10, 2019 •

edited

Loading

gvbgduh Oct 2, 2019 •

edited

Loading

gvbgduh Oct 3, 2019 •

edited

Loading

lovelydinosaur Sep 30, 2019 •

edited

Loading

lovelydinosaur commented Sep 30, 2019 •

edited

Loading

gvbgduh commented Feb 5, 2020 •

edited

Loading