Cockroachdb unimplemented feature multiple active portals not supported

[nagylzs]

• asyncpg version: 0.20.1
• Cockroachdb version: v20.1.0 (connected to a 5-node on premise cluster)
• Python version: 3.8.0
• Platform: Linux
• Do you use pgbouncer?: No
• Did you install asyncpg with pip?: Yes

Example code:

    async with pool.acquire() as conn:
        async with conn.transaction():
            if not await conn.table_exists("sys_app_config"):
                await conn.execute("""
                    CREATE TABLE sys_app_config(
                        key  text NOT NULL PRIMARY KEY,
                        value jsonb
                    );
                """)

The table_exists method looks like this:

class MyConnection(Connection):
    async def table_exists(self, table_name: str) -> bool:
        """Tell if a table exists."""
        return await self.fetchval(
            "select table_name from information_schema.tables where table_name=$1", table_name) is not None

This code is actually called from tornado.ioloop.IOLoop.current().run_sync so it is guaranteed that nothing else is running queries concurrently.

Here is the traceback that I'm getting:

 File "/home/user/my_project/db/initialize.py", line 19, in init_database
    if not await conn.table_exists("sys_app_config"):
  File "/home/user/.local/share/virtualenvs/backend-YgzYW8Ky/lib/python3.8/site-packages/asyncpg/transaction.py", line 91, in __aexit__
    await self.__commit()
  File "/home/user/.local/share/virtualenvs/backend-YgzYW8Ky/lib/python3.8/site-packages/asyncpg/transaction.py", line 179, in __commit
    await self._connection.execute(query)
  File "/home/user/.local/share/virtualenvs/backend-YgzYW8Ky/lib/python3.8/site-packages/asyncpg/connection.py", line 272, in execute
    return await self._protocol.query(query, timeout)
  File "asyncpg/protocol/protocol.pyx", line 316, in query
asyncpg.exceptions.FeatureNotSupportedError: unimplemented: multiple active portals not supported
HINT:  You have attempted to use a feature that is not yet implemented.
See: https://github.com/cockroachdb/cockroach/issues/40195

The referenced issue tells that it is not possible to keep multiple queries open and fetch from them interleaved.

However, I'm not doing anything that should result in opening multiple queries and fetching from them that way. As far as I see, my code should not open a new query before closing the previous one.

I suspect that asyncpg itself forgot to close something in the background.

[nagylzs]

I have uploaded a minimal working example here:

https://gist.github.com/nagylzs/acc716982f8a8213cbe5d6d6495e5387

[nagylzs]

Some more info: if I don't start a new transaction ( async with conn.transaction() ) then it works.

[vaperce]

Coud it be related to this issue from Cockroach cockroachdb/cockroach#42912?

[nagylzs]

It could be. Then it would mean that fetchval or execute did not close the resultset before returning the result? (I'm guessing - these are the only two queries opened.)

[vaperce]

From my analysis, Cockroach is unable to implicitly close a result set (named also portal) in a transaction which it should do if it was respecting completely PSQL spec.

As far as I understand, they are not willing to fix this anytime soon because it would require them to do a lot of refactoring and only a few clients are behaving like asyncpg.

In this example:

• fetchval opens a portal because it has arguments
• if the condition is False the next execute else the hidden "COMMIT;" when finishing transaction raises the error because Cockroach expects the portal to be closed before doing anything else

I'm working on a fix (for asyncpg)

[ale-dd]

@vaperce did you get a chance to patch asyncpg?

[vaperce]

Nope, didn't.

Ended up this portal issue was not the only one with Cockroach, some fixes in asyncpg/introspection.py would have been required to make it fully working (not even sure it is possible), so I decided to stop the dev at this step.

If you wanna continue it, please feel free to you use my work on the portals.

[rafiss]

Hi all, I work on cockroachdb. I tested with a bleeding edge version of cockroachdb (which will soon be released as v21.2-alpha).

The issue reported in #580 (comment) no longer occurs. I think cockroachdb/cockroach#63677 might have improved the situation.

Also, I ran the full asyncpg test suite against cockroachdb, and it went well:

Test failed: <unittest.runner.TextTestResult run=284 errors=0 failures=2>
error: Test failed: <unittest.runner.TextTestResult run=284 errors=0 failures=2>

The two failures are test_connect_pgpass_regular and test_connect_params and they failed because the default port differs from PostgreSQL.

[hifi]

This is still an issue, tested with CockroachDB v21.2.5 and asyncpg 0.25.0 with a minimal test case:

import asyncio
import asyncpg

async def main():
    conn = await asyncpg.connect('postgresql://root@localhost:26257/defaultdb?sslmode=disable')

    async with conn.transaction():
        print(await conn.fetchrow("SELECT 1"))
        print(await conn.fetchrow("SELECT 2"))
    
    await conn.close()

asyncio.get_event_loop().run_until_complete(main())

Results in:

<Record ?column?=1>
Traceback (most recent call last):
  File "/home/hifi/git/asyncpg-cockroachdb/test.py", line 13, in <module>
    asyncio.get_event_loop().run_until_complete(main())
  File "/usr/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/home/hifi/git/asyncpg-cockroachdb/test.py", line 9, in main
    print(await conn.fetchrow("SELECT 2"))
  File "/home/hifi/git/asyncpg-cockroachdb/venv/lib/python3.9/site-packages/asyncpg/connection.py", line 679, in fetchrow
    data = await self._execute(
  File "/home/hifi/git/asyncpg-cockroachdb/venv/lib/python3.9/site-packages/asyncpg/connection.py", line 1659, in _execute
    result, _ = await self.__execute(
  File "/home/hifi/git/asyncpg-cockroachdb/venv/lib/python3.9/site-packages/asyncpg/connection.py", line 1684, in __execute
    return await self._do_execute(
  File "/home/hifi/git/asyncpg-cockroachdb/venv/lib/python3.9/site-packages/asyncpg/connection.py", line 1711, in _do_execute
    stmt = await self._get_statement(
  File "/home/hifi/git/asyncpg-cockroachdb/venv/lib/python3.9/site-packages/asyncpg/connection.py", line 398, in _get_statement
    statement = await self._protocol.prepare(
  File "asyncpg/protocol/protocol.pyx", line 168, in prepare
asyncpg.exceptions.FeatureNotSupportedError: unimplemented: multiple active portals not supported
DETAIL:  cannot perform operation sql.PrepareStmt while a different portal is open
HINT:  You have attempted to use a feature that is not yet implemented.
See: https://go.crdb.dev/issue-v/40195/v21.2

@rafiss anything you could think of to help this case?

[hifi]

I have zeroed in on this to the fact that fetchrow/fetchval (and various other) functions set the row limit to 1 for these pgwire requests and CockroachDB handles it somehow incorrectly as setting the limit to 0 works around it completely without any visible issues and the queries will still only fetch a single result but the transaction will continue working.

[jordanlewis]

Hi, I'm also a maintainer for CockroachDB.

The root cause of this is that Postgres permits leaving cursors open and beginning new cursors. CockroachDB does not.

The problem here is that asyncpg doesn't explicitly close portals when it's done using them. The example code from @hifi is correct in Postgres, but it actually leads to cursor leakage: if you run this against Postgres, you won't get errors, but you will have leaked server-side cursors until you close your connection.

This is bad for performance in Postgres, and just plain doesn't work in CockroachDB.

I think the best solution would be for asyncpg to provide a cursor.close() method that permits explicit closure of cursors. A call like conn.fetchRow should close the cursor it opens under the hood before exiting.

I believe this will improve performance in Postgres, and make the library compatible with CockroachDB.

[elprans]

The problem here is that asyncpg doesn't explicitly close portals when it's done using them

asyncpg uses an unnamed portal in fetch and other methods. Postgres does not require it to be destroyed explicitly, as unnamed portals get auto-destroyed on the next Bind. It seems to me that Cockroach should follow the unnamed portal semantics to be properly wire-compatible?

but it actually leads to cursor leakage

It doesn't, asyncpg closes prepared statements when the statement object is garbage collected.

[jordanlewis]

Thank you for the reply!

as unnamed portals get auto-destroyed on the next Bind. It seems to me that Cockroach should follow the unnamed portal semantics to be properly wire-compatible?

You're right, CockroachDB indeed doesn't do this properly. Thanks for calling that out, I missed that asyncpg uses the unnamed portal for this purpose. We will correct this error.

It doesn't, asyncpg closes prepared statements when the statement object is garbage collected

But what about cursors, which are connected to portals (different from prepared statements)?

[elprans]

But what about cursors, which are connected to portals (different from prepared statements)?

They also get closed on garbage collection. We can implement an explicit .close() method, though.

[jordanlewis]

They also get closed on garbage collection.

I could be missing something, but I was looking around at the code and I don't see where we ever invoke CoreProtocol._close with is_portal=true - doesn't that mean we never actually explicitly close any portals?

We can implement an explicit .close() method, though.

I think that would be useful. It's good that they get closed when GC'd, but isn't it true then that the Postgres server will have to hold open the cursors for an arbitrary amount of time, depending on how the client happens to be behaving with respect to GC? I'm not familiar with how Python GC works, to be fair, but I would be nervous to tie server-side resources to client-side GC behavior.

[elprans]

doesn't that mean we never actually explicitly close any portals?

Hmm, you're right, this could be an issue.

[rafiss]

asyncpg uses an unnamed portal in fetch and other methods. Postgres does not require it to be destroyed explicitly, as unnamed portals get auto-destroyed on the next Bind. It seems to me that Cockroach should follow the unnamed portal semantics to be properly wire-compatible?

I believe since cockroachdb/cockroach#76792 (in v22.1.0), CockroachDB should implement that behavior. I'm not able to confirm at the moment.

[rafiss]

Actually I was mistaken above.

However, I created cockroachdb/cockroach#83164, which should implement the correct unnamed portal behavior for the specific way that asyncpg uses them (sending a Prepare, then Describe, then Bind of an unnamed portal). A more general fix is complicated, but this one can at least be backported to existing CockroachDB versions.

It still seems useful to implement the close() function as described above though, which is a more bulletproof solution.

[rimadeodhar]

For the above code snippet by @hifi, one workaround that I noticed is that the error goes away by replace fetchval with fetch. By examining the difference over Wireshark between the two calls, I noticed since fetch results in fetching all rows, the execution step completes with a CommandComplete message as opposed to the PortalSuspended message by using fetchval.
This doesn't obviate the need for a general fix on the cockroachdb side though as we should still follow the unnamed portal semantics to be properly wire-compatible.