sql: add extra information to protocol errors in bind by cucaroach · Pull Request #106130 · cockroachdb/cockroach

cucaroach · 2023-07-04T22:51:20Z

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 · 2023-07-04T22:51:28Z

This change is

knz · 2023-07-05T20:40:16Z

pkg/sql/conn_executor_prepare.go

-	retErr := func(err error) (fsm.Event, fsm.EventPayload) {
+	retErr1 := func(err error) (fsm.Event, fsm.EventPayload) {
+		if bindCmd.PreparedStatementName != "" {
+			err = errors.Wrapf(err, "bind prepared statement %q failed", bindCmd.PreparedStatementName)


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

knz · 2023-07-05T20:40:22Z

pkg/sql/conn_executor_prepare.go

+
+	retErr := func(err error) (fsm.Event, fsm.EventPayload) {
+		if ps.StatementSummary != "" {
+			err = errors.Wrapf(err, "bind statement (%q) failed", ps.StatementSummary)


cucaroach · 2023-07-06T15:54:43Z

@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)

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

cucaroach · 2023-07-17T12:56:03Z

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 · 2023-08-05T00:38:42Z

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 · 2023-08-08T18:01:29Z

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 · 2023-08-08T19:10:06Z

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 · 2023-08-08T19:26:54Z

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 · 2023-08-08T19:27:38Z

bors r=rafiss

craig · 2023-08-08T19:55:27Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-08-08T22:19:42Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-08-08T22:19:46Z

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

The backport branch might have already existed.
There was a merge conflict.
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.}

cucaroach force-pushed the proto-err-detail branch from 79b7f23 to f6930cf Compare July 5, 2023 16:41

cucaroach marked this pull request as ready for review July 5, 2023 16:41

cucaroach requested review from a team as code owners July 5, 2023 16:41

cucaroach requested a review from rafiss July 5, 2023 16:42

knz reviewed Jul 5, 2023

View reviewed changes

cucaroach requested review from a team and rytaft July 6, 2023 13:38

cucaroach force-pushed the proto-err-detail branch from f6930cf to ca0ae39 Compare July 6, 2023 15:49

rafiss reviewed Jul 6, 2023

View reviewed changes

cucaroach force-pushed the proto-err-detail branch 3 times, most recently from d8e157b to 6f9a637 Compare July 12, 2023 13:07

cucaroach force-pushed the proto-err-detail branch from 6f9a637 to ee3a9ab Compare July 15, 2023 13:03

rafiss approved these changes Aug 8, 2023

View reviewed changes

cucaroach added the backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only label Aug 8, 2023

craig bot merged commit 0d110cd into cockroachdb:master Aug 8, 2023

This was referenced Aug 9, 2023

pgwire: add extra information to protocol errors in bind #108459

Closed

release-23.1: pgwire: add extra information to protocol errors in bind #108462

Closed

cucaroach mentioned this pull request Aug 10, 2023

release-23.1: sql: add extra information to protocol errors in bind #108453

Closed

	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,
	})

Conversation

cucaroach commented Jul 4, 2023

Uh oh!

cockroach-teamcity commented Jul 4, 2023

Uh oh!

knz Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

cucaroach Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

knz Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

cucaroach Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

cucaroach commented Jul 6, 2023

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

cucaroach commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuzefovich commented Aug 5, 2023

Uh oh!

cucaroach commented Aug 8, 2023

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss commented Aug 8, 2023

Uh oh!

cucaroach commented Aug 8, 2023

Uh oh!

cucaroach commented Aug 8, 2023

Uh oh!

craig bot commented Aug 8, 2023

Uh oh!

craig bot commented Aug 8, 2023

Uh oh!

blathers-crl bot commented Aug 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cucaroach commented Jul 17, 2023 •

edited

Loading