Skip to content

Cockroachdb connection pooling #87

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

Closed
kszucs opened this issue Mar 14, 2017 · 18 comments
Closed

Cockroachdb connection pooling #87

kszucs opened this issue Mar 14, 2017 · 18 comments

Comments

@kszucs
Copy link

kszucs commented Mar 14, 2017

  • asyncpg version: 0.9.0
  • PostgreSQL version: Cockroachdb beta-20170223
  • Python version: Python 3.6.0
  • Platform: latest OS X
  • Do you use pgbouncer?: no
  • Did you install asyncpg with pip?: yes
  • If you built asyncpg locally, which version of Cython did you use?: 0.25.2
  • Can the issue be reproduced under both asyncio and
    uvloop?
    : not related

I'm trying to use cockroachdb with asyncpg.

With plain connection it works like a charm. On the other hand using connection pool fails with the following error:

ERROR:sanic:Traceback (most recent call last):
  File "/Users/krisz/.pyenv/versions/miniconda3-latest/envs/ml3/lib/python3.6/site-packages/sanic/app.py", line 391, in handle_request
    response = await response
  File "/Users/krisz/Workspace/papi/papi/app.py", line 34, in accounts_create
    print(results)
  File "/Users/krisz/.pyenv/versions/miniconda3-latest/envs/ml3/lib/python3.6/site-packages/asyncpg/pool.py", line 257, in __aexit__
    await self.pool.release(con)
  File "/Users/krisz/.pyenv/versions/miniconda3-latest/envs/ml3/lib/python3.6/site-packages/asyncpg/pool.py", line 189, in release
    await connection.reset()
  File "/Users/krisz/.pyenv/versions/miniconda3-latest/envs/ml3/lib/python3.6/site-packages/asyncpg/connection.py", line 412, in reset
    ''')
  File "/Users/krisz/.pyenv/versions/miniconda3-latest/envs/ml3/lib/python3.6/site-packages/asyncpg/connection.py", line 171, in execute
    return await self._protocol.query(query, timeout)
  File "asyncpg/protocol/protocol.pyx", line 255, in query (asyncpg/protocol/protocol.c:56799)
asyncpg.exceptions.InternalServerError: syntax error at or near "DO"

Caused by the resetting the connection :

await self.execute('''
            DO $$
            BEGIN
                PERFORM * FROM pg_listening_channels() LIMIT 1;
                IF FOUND THEN
                    UNLISTEN *;
                END IF;
            END;
            $$;
            SET SESSION AUTHORIZATION DEFAULT;
            RESET ALL;
            CLOSE ALL;
            SELECT pg_advisory_unlock_all();
        ''')

I don't know the exact solution, but I'd like to use Cockroach with asyncpg :)

I've created an issue for cockroach developers too cockroachdb/examples-orms#19

@kszucs
Copy link
Author

kszucs commented Mar 14, 2017

UPDATE: Cockroach doesn't support plpgsql. Is there a way to use asyncpg's connection pooling without plpgsql scripts?

@elprans
Copy link
Member

elprans commented Mar 14, 2017

We could put a check there if cocroach distinguishes itself somehow through its version string. What does connection.get_server_version() return?

@kszucs
Copy link
Author

kszucs commented Mar 14, 2017

ServerVersion(major=9, minor=5, micro=0, releaselevel='final', serial=0)

@elprans
Copy link
Member

elprans commented Mar 14, 2017

That seems like a stock Postgres response. We need some way to tell that it's not Postgres on the other end.

@kszucs
Copy link
Author

kszucs commented Mar 14, 2017

cc @knz @tamird

@kszucs
Copy link
Author

kszucs commented Mar 14, 2017

I couldn't find suitable statement: https://www.cockroachlabs.com/docs/sql-statements.html
Here is their sqlalchemy adapter/dialect

How about explicitly telling to the pool with a parameter?

@knz
Copy link

knz commented Mar 14, 2017

Also perhaps cockroachdb/cockroach#14142 would help? (once solved)

@elprans
Copy link
Member

elprans commented Mar 14, 2017

How about explicitly telling to the pool with a parameter?

I don't think it's a good idea.

@elprans
Copy link
Member

elprans commented Mar 14, 2017

Also perhaps cockroachdb/cockroach#14142 would help? (once solved)

Yes, anything on SQL level that does not raise an error under Postgres would be fine.

@elprans
Copy link
Member

elprans commented Mar 14, 2017

@knz Ideally, though, Cocroach should send something in the ParameterStatus message (in addition to server_version).

@knz
Copy link

knz commented Mar 14, 2017

@elprans do you reckon (other) clients would be confused by receiving a parameterstatus key that's not provided by pg otherwise?

@elprans
Copy link
Member

elprans commented Mar 14, 2017

@knz I doubt that. Postgres itself adds new keys periodically: https://www.postgresql.org/docs/current/static/protocol-flow.html#PROTOCOL-ASYNC

@elprans
Copy link
Member

elprans commented Mar 14, 2017

Relevant excerpt:

This set might change in the future, or even become configurable. Accordingly, a frontend should simply ignore ParameterStatus for parameters that it does not understand or care about.

knz added a commit to knz/cockroach that referenced this issue Mar 14, 2017
…ssage.

Discussed in MagicStack/asyncpg#87 (comment)

It is useful for clients to know that they are actually talking to a
CockroachDB SQL node, as opposed to a real PostgreSQL server. At this
point there is no way to determine a client is connected to
CockroachDB using a SQL statement/query that would not otherwise
*fail* in PostgreSQL.

This patch addresses this limitation by returning a new `crdb_version`
parameter in the ParameterStatus
message. (https://www.postgresql.org/docs/9.5/static/protocol-message-formats.html)

This is safe because as explained in
https://www.postgresql.org/docs/9.5/static/protocol-flow.html#PROTOCOL-ASYNC
"This set might change in the future, or even become
configurable. Accordingly, a frontend should simply ignore
ParameterStatus for parameters that it does not understand or care
about."
@knz
Copy link

knz commented Mar 14, 2017

FYI cockroachdb/cockroach#14145

knz added a commit to knz/cockroach that referenced this issue Mar 14, 2017
…ssage.

Discussed in MagicStack/asyncpg#87 (comment)

It is useful for clients to know that they are actually talking to a
CockroachDB SQL node, as opposed to a real PostgreSQL server. At this
point there is no way to determine a client is connected to
CockroachDB using a SQL statement/query that would not otherwise
*fail* in PostgreSQL.

This patch addresses this limitation by returning a new `crdb_version`
parameter in the ParameterStatus
message. (https://www.postgresql.org/docs/9.5/static/protocol-message-formats.html)

This is safe because as explained in
https://www.postgresql.org/docs/9.5/static/protocol-flow.html#PROTOCOL-ASYNC
"This set might change in the future, or even become
configurable. Accordingly, a frontend should simply ignore
ParameterStatus for parameters that it does not understand or care
about."
knz added a commit to knz/cockroach that referenced this issue Mar 14, 2017
…ssage.

Discussed in MagicStack/asyncpg#87 (comment)

It is useful for clients to know that they are actually talking to a
CockroachDB SQL node, as opposed to a real PostgreSQL server. At this
point there is no way to determine a client is connected to
CockroachDB using a SQL statement/query that would not otherwise
*fail* in PostgreSQL.

This patch addresses this limitation by returning a new `crdb_version`
parameter in the ParameterStatus
message. (https://www.postgresql.org/docs/9.5/static/protocol-message-formats.html)

This is safe because as explained in
https://www.postgresql.org/docs/9.5/static/protocol-flow.html#PROTOCOL-ASYNC
"This set might change in the future, or even become
configurable. Accordingly, a frontend should simply ignore
ParameterStatus for parameters that it does not understand or care
about."
@kszucs
Copy link
Author

kszucs commented Mar 14, 2017

Great! 👍 Really appreciate your work!

@knz
Copy link

knz commented Mar 14, 2017

The ParameterStatus is crdb_version -- you'll get it if you build CockroachDB from source now, or wait until the next beta release later this week.

@kszucs
Copy link
Author

kszucs commented Mar 14, 2017

@elprans I've built a docker image kszucs/cockroach:20170314

elprans added a commit that referenced this issue Mar 16, 2017
Add basic server capability detection mechanism based on server version
and parameters reported by the server through ParameterStatus messages.
This allows altering certain asyncpg behaviour based on the connected
server.

Specifically, this allows asyncpg to connect to CochroachDB servers.

Fixes #87.
elprans added a commit that referenced this issue Mar 16, 2017
Add basic server capability detection mechanism based on server version
and parameters reported by the server through ParameterStatus messages.
This allows altering certain asyncpg behaviour based on the connected
server.

Specifically, this allows asyncpg to connect to CochroachDB servers.

Fixes #87.
elprans added a commit that referenced this issue Mar 16, 2017
Add basic server capability detection mechanism based on server version
and parameters reported by the server through ParameterStatus messages.
This allows altering certain asyncpg behaviour based on the connected
server.

Specifically, this allows asyncpg to connect to CochroachDB servers.

Fixes #87.
elprans added a commit that referenced this issue Mar 17, 2017
Add basic server capability detection mechanism based on server version
and parameters reported by the server through ParameterStatus messages.
This allows altering certain asyncpg behaviour based on the connected
server.

Specifically, this allows asyncpg to connect to CochroachDB servers.

Fixes #87.
elprans added a commit that referenced this issue Mar 17, 2017
Add basic server capability detection mechanism based on server version
and parameters reported by the server through ParameterStatus messages.
This allows altering certain asyncpg behaviour based on the connected
server.

Specifically, this allows asyncpg to connect to CochroachDB servers.

Fixes #87.
@kszucs
Copy link
Author

kszucs commented Mar 18, 2017

Great, thanks!

mfussenegger added a commit to crate/crate that referenced this issue Nov 3, 2017
For clients it may be useful to know that they're communicating with
CrateDB instead of a PostgreSQL instance.
For example to adapt the server capabilities.

One such client is asyncpg, see MagicStack/asyncpg#87
mfussenegger added a commit to crate/crate that referenced this issue Nov 3, 2017
For clients it may be useful to know that they're communicating with
CrateDB instead of a PostgreSQL instance.
For example to adapt the server capabilities.

One such client is asyncpg, see MagicStack/asyncpg#87
mfussenegger added a commit to crate/crate that referenced this issue Nov 3, 2017
For clients it may be useful to know that they're communicating with
CrateDB instead of a PostgreSQL instance.
For example to adapt the server capabilities.

One such client is asyncpg, see MagicStack/asyncpg#87
mcuadros pushed a commit to mcuadros/pgwire that referenced this issue Feb 3, 2018
…ssage.

Discussed in MagicStack/asyncpg#87 (comment)

It is useful for clients to know that they are actually talking to a
CockroachDB SQL node, as opposed to a real PostgreSQL server. At this
point there is no way to determine a client is connected to
CockroachDB using a SQL statement/query that would not otherwise
*fail* in PostgreSQL.

This patch addresses this limitation by returning a new `crdb_version`
parameter in the ParameterStatus
message. (https://www.postgresql.org/docs/9.5/static/protocol-message-formats.html)

This is safe because as explained in
https://www.postgresql.org/docs/9.5/static/protocol-flow.html#PROTOCOL-ASYNC
"This set might change in the future, or even become
configurable. Accordingly, a frontend should simply ignore
ParameterStatus for parameters that it does not understand or care
about."
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants