sql: add extra information to protocol errors in bind

[cucaroach]

A user is running into mysterious protocol errors when using prepared
statements. Add some extra information to the error message to help
guide the investigation.

Informs: https://github.com/cockroachlabs/support/issues/2184
Release note: none
Epic: none

[cockroach-teamcity]

This change is 

[knz]

nit: pls consider errors.WithDetail instead of copying the full SQL syntax as a prefix.

[cucaroach]

👍

[knz]

ditto

[cucaroach]

👍

[cucaroach]

@rafiss @knz what I can't figure out is how I get this error message into the logs. Nothing I've tried gets this error message into the crdb logs. I'm running a PG test with the server running and I tried sql.trace.session_eventlog.enabled, server.auth_log.sql_sessions.enabled, server.auth_log.sql_connections.enabled, sql.trace.log_statement_execute and nothing seems to work. Any help/ideas would be most welcome!

Running test like this:

./dev test pkg/sql/pgwire -f TestPGTest/bind_proto_error --test-args "-addr localhost:26257 -user root" -v

Where bind_proto_error just contains the new test I added to errors to hit this error path. Ideally we'd just roll this patch into an upgrade and error message would show up in logs the next time it gets hit.

[rafiss]

The Detail field of an ErrorResponse is separate from the Message field. See:

cockroach/pkg/testutils/pgtest/pgtest.go

Lines 199 to 217 in d04dde5

	if errmsg, ok := recv.(*pgproto3.ErrorResponse); ok {
	if typ != typErrorResponse {
	return nil, errors.Errorf("waiting for %T, got %#v", typs[0], errmsg)
	}
	var message, detail, hint string
	if keepErrMsg {
	message = errmsg.Message
	detail = errmsg.Detail
	hint = errmsg.Hint
	}
	// ErrorResponse doesn't encode/decode correctly, so
	// manually append it here.
	msgs = append(msgs, &pgproto3.ErrorResponse{
	Code: errmsg.Code,
	Message: message,
	ConstraintName: errmsg.ConstraintName,
	Detail: detail,
	Hint: hint,
	})

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @knz, and @rytaft)

---

pkg/sql/conn_executor_prepare.go line 349 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

👍

nit: the word "failed" is probably redundant in the detail message

---

pkg/sql/conn_executor_prepare.go line 364 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

👍

nit: i would say statement summary instead of statement, so that this message is distinguished from the one above

---

pkg/sql/conn_executor_prepare.go line 362 at r2 (raw file):

	}

	retErr := func(err error) (fsm.Event, fsm.EventPayload) {

nit: i'd prefer not to have two retErr functions. could we just add this to the first one above:

		if ps != nil && ps.StatementSummary != "" {
			err = errors.WithDetailf(err, "statement (%q) failed", ps.StatementSummary)
		}

(and add a var ps *PreparedStatement declaration above)

[cucaroach]

This is RFAL, I added a log statement where the mysterious error is occurring, these errors are logged by default and I wanted to be able to search for these errors in logs. Maybe there's a better way? Ping @rafiss @knz hoping to get this into next 23.1 release to finally get to bottom of lingering customer issue.

[yuzefovich]

We got the same support ticket again over in https://github.com/cockroachlabs/support/issues/2510 - should we push this over the finish line and backport? @cucaroach @rafiss @knz

[cucaroach]

I think so. I think I addressed all the review feedback so just waiting on review approval.

[rafiss]

Reviewed 2 of 7 files at r1, 4 of 5 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @knz, and @rytaft)

[rafiss]

yeah, i think we should try to get this out. thanks for your improvements @cucaroach, and @yuzefovich for following up

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @knz, and @rytaft)

[rafiss]

general tip: if you click the "re-request review" button, then Github reminders would kick in to prod me to review this so you don't have to :)

[cucaroach]

general tip: if you click the "re-request review" button, then Github reminders would kick in to prod me to review this so you don't have to :)

TIL! Thanks!

[cucaroach]

bors r=rafiss

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from ee3a9ab to blathers/backport-release-23.1-106130: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}