sql/pgwire: implicitly destroy portal on txn finish

[rafiss]

fixes #42912
touches #40195

This also includes a commit to fix an error message hint that seems to be
accidentally using WithSafeDetails, which is meant for sentry reporting.
(Originally added in #40197.)

Release note (sql change): Previously, committing a transaction when a
portal was suspended would cause a "multiple active portals not
supported" error. Now, the portal is automatically destroyed.

[cockroach-teamcity]

This change is 

[jordanlewis]

I feel like we would do well to add a pgjdbc test for this as well to exercise the cases that we've gotten in the issues. Unless this will unlock some test cases in their test suite?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/pgwire/command_result.go, line 499 at r2 (raw file):

			telemetry.Inc(sqltelemetry.InterleavedPortalRequestCounter)
			return errors.WithDetail(sql.ErrLimitedResultNotSupported,
				fmt.Sprintf("cannot perform operation %T while a different portal is open", c))

If you get rid of the errors.Safe thing, doesn't the type get redacted from logs?

[rafiss]

will add a pgjdbc test good call!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)

---

pkg/sql/pgwire/command_result.go, line 499 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

If you get rid of the errors.Safe thing, doesn't the type get redacted from logs?

in this case no, because WithDetail is just meant for the hint that is returned to the end user, and also it's just a naked string (that's why i changed to fmt.Sprintf).

WithSafeDetails on the other hand, is meant for sentry/reporting, and does require format args and need to specify when an argument is safe for reporting.

[rafiss]

well, turns out pgjdbc always sends a COMMIT using the extended protocol. so it does in fact rely on multiple active portals, and supporting that will require fully implementing #40195

but the flow that i added to pgtest is the one used by the rust driver, so this change still seems useful

bors r=jordanlewis

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)

[rafiss]

need to fix one of the tests

bors r-

[craig]

Canceled.

[jordanlewis]

So, this does not in fact fix #42912 after all? I'm confused 🤔

[rafiss]

Yes, it does fix #42912. Previously, the following commands would fail at the COMMIT command.

send
Query {"String": "BEGIN"}
----

until
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"BEGIN"}
{"Type":"ReadyForQuery","TxStatus":"T"}

# 83 = ASCII 'S' for Statement
# 49 = ASCII '1'
# ParameterFormatCodes = [0] for text format
send
Parse {"Name": "s4", "Query": "select n::int4 from generate_series(0,$1::int8) n", "ParameterOIDs": [20]}
Describe {"ObjectType": 83, "Name": "s4"}
Bind {"PreparedStatement": "s4", "ParameterFormatCodes": [0], "ResultFormatCodes": [0], "Parameters": [[49]]}
Execute {"MaxRows": 1}
Sync
----

until
ReadyForQuery
----
{"Type":"ParseComplete"}
{"Type":"ParameterDescription","ParameterOIDs":[20]}
{"Type":"RowDescription","Fields":[{"Name":"n","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":23,"DataTypeSize":4,"TypeModifier":-1,"Format":0}]}
{"Type":"BindComplete"}
{"Type":"DataRow","Values":[{"text":"0"}]}
{"Type":"PortalSuspended"}
{"Type":"ReadyForQuery","TxStatus":"T"}


send
Query {"String": "COMMIT"}
----

until
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"COMMIT"}
{"Type":"ReadyForQuery","TxStatus":"I"}

This is what the Rust sqlx driver does, according to this user-provided pcap: #40195 (comment)

This works now after this change.

We can't test this with PGJDBC, because PGJDBC always executes COMMIT using the extended protocol. In other words, instead of sending a COMMIT like this:

send
Query {"String": "COMMIT"}
----

PGJDBC sends a COMMIT like this:

send
Parse {"Name": "commit_stmt", "Query": "COMMIT"}
Bind {"PreparedStatement": "commit_stmt"}
Execute
Sync
----

[rafiss]

@jordanlewis thanks for the nudge -- turns out we actually can handle a "prepared commit" also. Added that, and a PGJDBC test.

[rafiss]

@jordanlewis this is ready for another look

[jordanlewis]

woo hoo!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/pgwire/command_result.go, line 497 at r4 (raw file):

// Parse/Bind/Execute sequence for a COMMIT query.
func (r *limitedCommandResult) isCommit() (bool, error) {
	cmd, _, err := r.conn.stmtBuf.CurCmd()

nit: I think a comment before each "major case" would be useful, to explain what the things are being checked. like, up top:

// Case 1: the statement is a COMMIT in simple protocol

then down below

// Case 2: the statement is a Parse/Bind/Execute sequence

otherwise it is a little hard to discern "the point" of what is going on with all of the case checking.

[rafiss]

thanks for the review! i've added comments at each case

bors r=jordanlewis

[craig]

Build failed:

• GitHub CI (Cockroach)

[rafiss]

bors r=jordanlewis

[rafiss]

bors r-

[rafiss]

bors r=jordanlewis

[craig]

Build failed:

• GitHub CI (Cockroach)

[rafiss]

bors r=jordanlewis

[craig]

Build succeeded:

• GitHub CI (Cockroach)