feat: asyncpg adapter
#177
Conversation
|
i just merged my commit, can you rebase? |
|
i can't really compare the two implementations like this, can you redo it so that you just rewrite the existing postgres adapter? |
Yep, can do. I'll also bring in your updated changes. |
| min_size: minimum pool size. (default 4) | ||
| The minimum number of Postgres connections. | ||
| max_size: maximum pool size. (default 20) | ||
| If greater than 0, this limits the maximum number of connections to Postgres. | ||
| Otherwise, maintain `min_size` number of connections. |
There was a problem hiding this comment.
Moved to the from_url method since it can't be modified after initial creation.
|
I've made the updates for the group and priority settings. I think it's ready for you to take a look at this point. |
| ) | ||
| result = await cursor.fetchone() | ||
| self.assertIsNone(result) | ||
|
|
||
| async def test_cron_job_close_to_target(self) -> None: |
There was a problem hiding this comment.
This test is a bit flaky. It passes about 50-60% of the time for me.
| ) | ||
| ).format( | ||
| jobs_table=self.jobs_table, | ||
| expire_at=SQL(",expire_at = %(expire_at)s" if expire_at != -1 else ""), |
There was a problem hiding this comment.
This is cool syntax sugar I wasn't aware of from psycopg.
Since there isn't a direct equivalent to this, the easiest thing to do was to break this into 2 statements.
|
benchmarks got 8.5x slower, it needs to be fixed before this is considered, benchmarks/simple.py saq_pg |
|
i'm also not a big fan of the api of asyncio compared to psycopg, looks like it's not dbapi. given the benchmarks, i'm so far not inclined to accept it, considering lgpl v3 only means users cannot modify the source, it doesn't seem to be that detrimental, but i do understand some companies cannot adhere to it. |
|
That's quite a surprising difference on performance. I definitely would not have expected that great of a difference. Re: the dbapi comment, you are correct. It does not have a dbapi compatible interface. Given this difference, do you want me to spend time investigating the performance gap? |
|
@cofin i've been thinking about it a bit and i apologize but i don't think i have much appetite for this at the moment, so i think we can close this for now. sorry about that and thanks for the effort. |
This PR implements the
asyncpgadapter mentioned in #172I've left the current
psycopgadapter in as a reference, but I can remove (if needed) based on feedback here.