Portal not being implicitly destroyed on transaction finish

[jmf-tls]

Describe the problem
Using to run a query in Java: when the query is inside a transaction with fetch size set, a cursor/portal is created on the server.
Upon committing the transaction without closing the ResultSet, the follwing error occurs:

org.postgresql.util.PSQLException: ERROR: 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

To Reproduce
Run CockroachTest
(the test is "standalone" and needs to have a CockroachDB or PostgreSQL container running; the commands used to start the containers are shown in the test comments)

Expected behavior
The test has 4 cases: a SELECT * query is ran using fetch size 0 or 2 (which is less than the size of the ResultSet); in addition, the query may run with or without a transaction.

When running without transaction, the fetch size is ignored and no problems occur (I suppose this is due to the limitations that also apply to PostgreSQL, using "fetch size" needs server cursors which only live for the duration of a transaction).

When running fetch size 0, the feature is disabled and no problems occur.

When running fetch size 2:

• CockroachDB 19.1.X fails due to sql/pgwire: missing support for row count limits in pgwire #4035 when running the query (feature not supported, was implemented only in 19.2.X).
• CockroachDB 19.2.X is able to run the query and return results, but fails on commit due to pgwire: multiple active result sets (portals) not supported #40195; however, if the ResultSet is closed before transaction commit, then no problem occurs.
• PostgreSQL 9.6.X passes the test without any problems.

Even though it is a good practice to close the ResultSets (as well as other Closeable objects), the patch in #33423 leads me to believe that the behavior to expect is the same as PostgreSQL: the cursors/portals should be implicitly closed when the transaction is done.

Environment:
CockroachDB 19.1.X, 19.2.X or PostgreSQL 9.6.X (or newer)

[jordanlewis]

Thanks for the report!

@mjibson is this the expected behavior? I can't quite remember.

[maddyblue]

This is the expected behavior. Our current implementation has known limitations. One of them is that it doesn't process any query messages. So even in this case, where the portal is implicitly closed, our implementation doesn't read ahead to check that it's a COMMIT statement.

"Why not just have it look ahead and see if the first statement in the next query is COMMIT?"

While we could maybe do this, this is exactly where our current implementation gets very hard to do correctly. We knew all of this going in, and you have hit exactly the point at which we decided to draw a line to avoid a great deal of complication.

We will however keep track of this issue and problem. If more people start to report it we may consider increasing the priority of this feature. (By "this feature" I mean supporting exactly this use case where there's a txn ending with an open portal.)

[andreimatei]

Would it really be hard to support this? The way I'd imagine this to work is to pop out limitedCommandResult when we see anything but a ExecPortal, consider the query finished (so don't allow a further ExecPortal on it) and then have the portal be naturally destroyed at the end of the transaction.