Postgres: fetch_one/fetch_optional doesn't close opened server side cursor

[DXist]

Bug Description

Current implementation of fetch_one/fetch_optional for Postgres doesn't close the used database resource - the server side cursor.

It just waits for the expected single data row (for fetch_optional it's the case when query returns one row) and relies on multiple workarounds across the codebase that will handle PortalSuspended message. I copied one of the workarounds when I recently implemented the pipelined query execution feature.

The open unnamed cursor is problematic in CockroachDB because it doesn't support multiple database cursors and rejects the next query.

When I run the provided below snippet of async code against CockroachDB I get the following error:

thread 'db_transaction' panicked at 'called `Result::unwrap()` on an `Err` value: Database(PgDatabaseError { severity: Error, code: "0A000", message: "unimplemented: multiple active portals not supported", detail: Some("cannot perform operation sql.BindStmt while a different portal is open"), hint: Some("You have att
empted to use a feature that is not yet implemented.\nSee: https://go.crdb.dev/issue-v/40195/v22.1"), position: None, where: None, schema: None, table: None, column: None, data_type: None, constraint: None, file: Some("distsql_running.go"), line: Some(1181), routine: Some("init") })'

Expected behavior

fetch_one/fetch_optional should close the server side cursor it uses.

The implementation adds Close command to the pipeline - Bind, Execute, Close and Sync and waits for PortalSuspended and CloseComplete messages.

With this implementation it's possible to remove multiple workarounds across the code base that handle PortalSuspended message.

A piece of Postgres documentation for Close command:

The Close message closes an existing prepared statement or portal and releases resources. It is not an error to issue Close against a nonexistent statement or portal name. The response is normally CloseComplete, but could be ErrorResponse if some difficulty is encountered while releasing resources. Note that closing a prepared statement implicitly closes any open portals that were constructed from that statement.

Another option could be an alternative of fetch_one/fetch_optional (maybe fetch_all_return_one?) that fetches all results then does client side filtering and returns the first data row (or optional row). This option will require less database round trips and will not use protocol level result set limit but may waste more resources if the query actually returns more than one row.

Minimal Reproduction

    let pool: &PgPool = PgPool::connect(url).await.unwrap();

    let mut transaction = pool.begin().await.unwrap();

    #[allow(dead_code)]
    struct X {
        col1: i64,
    }

    sqlx::query_as!(X, "SELECT 1 as \"col1!\"")
        .fetch_one(&mut transaction)
        .await
        .unwrap();
    sqlx::query_as!(X, "SELECT 1 as \"col1!\"")
        .fetch_one(&mut transaction)
        .await
        .unwrap();
    transaction.commit().await.unwrap();

Related issues

There are multiple related issues with discussions or implemented workarounds:

• Fix bug for PostgreSQL if the statement has type holes #1363
• PG: PortalSuspended for array of composite types #1254
• Postgres: Unexpected PortalSuspended when binding domain type of composite type of domain type #1110
• CockroachDB related - Close open portals after receiving PortalSuspended #949

Info

• SQLx version: [0.6.1]
• SQLx features enabled: ["uuid", "migrate", "json", "runtime-tokio-rustls", "postgres", "offline"]
• Database server and version: [CockroachDB CCL v22.1.5 @ 2022/07/28 14:58:04 (go1.17.11)]
• Operating system: [Linux]
• rustc --version: [rustc 1.63.0 (4b91a6ea7 2022-08-08)]

[abonander]

#933 (comment)

[DXist]

fetch_one and fetch_optional can close the portal explicitly by issuing a Close command after Sync in the same pipelined batch.

The sqlx documention for fetch_one/fetch_optional says that "Extra rows are ignored". The client can deliver this fact to the database server and let it free the resource.

[DXist]

The unclosed portal affects Postgres too.

I found a discussion that shows the deadlock reproduced by the following test code.

In this test session c3 leaves the unnamed portal opened. 'VACUUM FREEZE' command in the concurrent session blocks:

"Further investigation showed it happens due to portal is not dropped
inside of exec_execute_message, and it is kept in third session till
COMMIT is called. Therefore heap page remains pinned, and VACUUM FREEZE
became locked inside LockBufferForCleanup."

The third session can't commit:

"But "freeze-the-dead" remains locked since third session could not
send COMMIT until VACUUM FULL finished."

The discussion resulted in adding a Close command to libpq to PQsendQuery in pipeline mode. Actually the Close command is issued after Execute and before Sync.

The first reply from the discussion:

I'm inclined to think that your complaint would be better handled
by having the client send a portal-close command, if it's not
going to do something else immediately.

[DXist]

There is a fix in the CockroachDB repo that implements unnamed portal drop behavior for Bind messages.

[abonander]

I'd accept a PR to include the Close command after the Execute here: https://github.com/launchbadge/sqlx/blob/main/sqlx-core/src/postgres/connection/executor.rs#L241

[DXist]

@abonander , I've made the PR

[abonander]

Closed by #2081