From e6710f68d166747d6e012b3972402b034cf0d690 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Sat, 12 Sep 2020 19:26:57 -0700 Subject: [PATCH 1/2] sql: set constraint name in pg error for FK violations This change adds infrastructure to set the "constraint name" field of errors on pgwire and uses it to populate the constraint name for foreign key violation errors. Fixes #36494. Release note (sql change): foreign key violation errors now fill in the "constraint name" error message field. --- pkg/cli/error.go | 6 +- pkg/sql/opt/exec/execbuilder/mutation.go | 8 +- pkg/sql/pgwire/conn.go | 9 +- pkg/sql/pgwire/pgerror/constraint_name.go | 94 ++++++++++++++++ .../pgwire/pgerror/constraint_name_test.go | 48 +++++++++ pkg/sql/pgwire/pgerror/errors.pb.go | 100 +++++++++++++----- pkg/sql/pgwire/pgerror/errors.proto | 1 + pkg/sql/pgwire/pgerror/flatten.go | 7 +- pkg/sql/pgwire/pgwirebase/msg.go | 17 +-- .../pgwirebase/servererrfieldtype_string.go | 14 ++- pkg/sql/pgwire/testdata/pgtest/errors | 76 +++++++++++++ pkg/testutils/lint/lint_test.go | 3 + pkg/testutils/pgtest/datadriven.go | 14 +-- pkg/testutils/pgtest/pgtest.go | 5 +- 14 files changed, 345 insertions(+), 57 deletions(-) create mode 100644 pkg/sql/pgwire/pgerror/constraint_name.go create mode 100644 pkg/sql/pgwire/pgerror/constraint_name_test.go create mode 100644 pkg/sql/pgwire/testdata/pgtest/errors diff --git a/pkg/cli/error.go b/pkg/cli/error.go index 30a255eea8a5..ba516c8384d3 100644 --- a/pkg/cli/error.go +++ b/pkg/cli/error.go @@ -69,12 +69,13 @@ func (f *formattedError) Error() string { severity := "ERROR" // Extract the fields. - var message, hint, detail, location string + var message, hint, detail, location, constraintName string var code pgcode.Code if pqErr := (*pq.Error)(nil); errors.As(f.err, &pqErr) { if pqErr.Severity != "" { severity = pqErr.Severity } + constraintName = pqErr.Constraint message = pqErr.Message code = pgcode.MakeCode(string(pqErr.Code)) hint, detail = pqErr.Hint, pqErr.Detail @@ -118,6 +119,9 @@ func (f *formattedError) Error() string { if detail != "" { fmt.Fprintln(&buf, "DETAIL:", detail) } + if constraintName != "" { + fmt.Fprintln(&buf, "CONSTRAINT:", constraintName) + } if hint != "" { fmt.Fprintln(&buf, "HINT:", hint) } diff --git a/pkg/sql/opt/exec/execbuilder/mutation.go b/pkg/sql/opt/exec/execbuilder/mutation.go index a4f75f95daf6..55f51cb6979f 100644 --- a/pkg/sql/opt/exec/execbuilder/mutation.go +++ b/pkg/sql/opt/exec/execbuilder/mutation.go @@ -776,11 +776,13 @@ func mkFKCheckErr(md *opt.Metadata, c *memo.FKChecksItem, keyVals tree.Datums) e referenced := md.TableMeta(c.ReferencedTable) var msg, details bytes.Buffer + var constraintName string if c.FKOutbound { // Generate an error of the form: // ERROR: insert on table "child" violates foreign key constraint "foo" // DETAIL: Key (child_p)=(2) is not present in table "parent". fk := origin.Table.OutboundForeignKey(c.FKOrdinal) + constraintName = fk.Name() fmt.Fprintf(&msg, "%s on table ", c.OpName) lex.EncodeEscapedSQLIdent(&msg, string(origin.Alias.ObjectName)) msg.WriteString(" violates foreign key constraint ") @@ -822,6 +824,7 @@ func mkFKCheckErr(md *opt.Metadata, c *memo.FKChecksItem, keyVals tree.Datums) e // "child_child_p_fkey" on table "child" // DETAIL: Key (p)=(1) is still referenced from table "child". fk := referenced.Table.InboundForeignKey(c.FKOrdinal) + constraintName = fk.Name() fmt.Fprintf(&msg, "%s on table ", c.OpName) lex.EncodeEscapedSQLIdent(&msg, string(referenced.Alias.ObjectName)) msg.WriteString(" violates foreign key constraint ") @@ -850,7 +853,10 @@ func mkFKCheckErr(md *opt.Metadata, c *memo.FKChecksItem, keyVals tree.Datums) e } return errors.WithDetail( - pgerror.Newf(pgcode.ForeignKeyViolation, "%s", msg.String()), + pgerror.WithConstraintName( + pgerror.Newf(pgcode.ForeignKeyViolation, "%s", msg.String()), + constraintName, + ), details.String(), ) } diff --git a/pkg/sql/pgwire/conn.go b/pkg/sql/pgwire/conn.go index fd80d6662efe..7cb6f1c7a42a 100644 --- a/pkg/sql/pgwire/conn.go +++ b/pkg/sql/pgwire/conn.go @@ -1252,15 +1252,20 @@ func writeErrFields( msgBuilder.writeTerminatedString(pgErr.Code) if pgErr.Detail != "" { - msgBuilder.putErrFieldMsg(pgwirebase.ServerErrFileldDetail) + msgBuilder.putErrFieldMsg(pgwirebase.ServerErrFieldDetail) msgBuilder.writeTerminatedString(pgErr.Detail) } if pgErr.Hint != "" { - msgBuilder.putErrFieldMsg(pgwirebase.ServerErrFileldHint) + msgBuilder.putErrFieldMsg(pgwirebase.ServerErrFieldHint) msgBuilder.writeTerminatedString(pgErr.Hint) } + if pgErr.ConstraintName != "" { + msgBuilder.putErrFieldMsg(pgwirebase.ServerErrFieldConstraintName) + msgBuilder.writeTerminatedString(pgErr.ConstraintName) + } + if pgErr.Source != nil { errCtx := pgErr.Source if errCtx.File != "" { diff --git a/pkg/sql/pgwire/pgerror/constraint_name.go b/pkg/sql/pgwire/pgerror/constraint_name.go new file mode 100644 index 000000000000..c294ffa716f0 --- /dev/null +++ b/pkg/sql/pgwire/pgerror/constraint_name.go @@ -0,0 +1,94 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package pgerror + +import ( + "context" + "fmt" + + "github.com/cockroachdb/errors" + "github.com/cockroachdb/errors/errorspb" + "github.com/gogo/protobuf/proto" +) + +// WithConstraintName decorates the error with a severity. +func WithConstraintName(err error, constraint string) error { + if err == nil { + return nil + } + + return &withConstraintName{cause: err, constraint: constraint} +} + +// GetConstraintName attempts to unwrap and find a Severity. +func GetConstraintName(err error) string { + if c := (*withConstraintName)(nil); errors.As(err, &c) { + return c.constraint + } + return "" +} + +type withConstraintName struct { + cause error + constraint string +} + +var _ error = (*withConstraintName)(nil) +var _ errors.SafeDetailer = (*withConstraintName)(nil) +var _ fmt.Formatter = (*withConstraintName)(nil) +var _ errors.SafeFormatter = (*withConstraintName)(nil) + +func (w *withConstraintName) Error() string { return w.cause.Error() } +func (w *withConstraintName) Cause() error { return w.cause } +func (w *withConstraintName) Unwrap() error { return w.cause } +func (w *withConstraintName) SafeDetails() []string { + // The constraint name is considered PII. + return nil +} + +func (w *withConstraintName) Format(s fmt.State, verb rune) { errors.FormatError(w, s, verb) } + +func (w *withConstraintName) SafeFormatError(p errors.Printer) (next error) { + if p.Detail() { + p.Printf("constraint name: %s", w.constraint) + } + return w.cause +} + +func encodeWithConstraintName(_ context.Context, err error) (string, []string, proto.Message) { + w := err.(*withConstraintName) + return "", nil, &errorspb.StringPayload{Msg: w.constraint} +} + +// decodeWithConstraintName is a custom decoder that will be used when decoding +// withConstraintName error objects. +// Note that as the last argument it takes proto.Message (and not +// protoutil.Message which is required by linter) because the latter brings in +// additional dependencies into this package and the former is sufficient here. +func decodeWithConstraintName( + _ context.Context, cause error, _ string, _ []string, payload proto.Message, +) error { + m, ok := payload.(*errorspb.StringPayload) + if !ok { + // If this ever happens, this means some version of the library + // (presumably future) changed the payload type, and we're + // receiving this here. In this case, give up and let + // DecodeError use the opaque type. + return nil + } + return &withConstraintName{cause: cause, constraint: m.Msg} +} + +func init() { + key := errors.GetTypeKey((*withConstraintName)(nil)) + errors.RegisterWrapperEncoder(key, encodeWithConstraintName) + errors.RegisterWrapperDecoder(key, decodeWithConstraintName) +} diff --git a/pkg/sql/pgwire/pgerror/constraint_name_test.go b/pkg/sql/pgwire/pgerror/constraint_name_test.go new file mode 100644 index 000000000000..1bace32c8cc0 --- /dev/null +++ b/pkg/sql/pgwire/pgerror/constraint_name_test.go @@ -0,0 +1,48 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package pgerror + +import ( + "context" + "fmt" + "testing" + + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/errors" + "github.com/stretchr/testify/require" +) + +func TestConstraintName(t *testing.T) { + testCases := []struct { + err error + expectedConstraint string + }{ + {WithConstraintName(fmt.Errorf("test"), "fk1"), "fk1"}, + {WithConstraintName(WithConstraintName(fmt.Errorf("test"), "fk1"), "fk2"), "fk2"}, + {WithConstraintName(WithCandidateCode(fmt.Errorf("test"), pgcode.FeatureNotSupported), "fk1"), "fk1"}, + {New(pgcode.Uncategorized, "i am an error"), ""}, + {WithCandidateCode(WithConstraintName(errors.Newf("test"), "fk1"), pgcode.System), "fk1"}, + {fmt.Errorf("something else"), ""}, + {WithConstraintName(fmt.Errorf("test"), "fk\"⌂"), "fk\"⌂"}, + } + + for _, tc := range testCases { + t.Run(tc.err.Error(), func(t *testing.T) { + constraint := GetConstraintName(tc.err) + require.Equal(t, tc.expectedConstraint, constraint) + // Test that the constraint name survives an encode/decode cycle. + enc := errors.EncodeError(context.Background(), tc.err) + err2 := errors.DecodeError(context.Background(), enc) + constraint = GetConstraintName(err2) + require.Equal(t, tc.expectedConstraint, constraint) + }) + } +} diff --git a/pkg/sql/pgwire/pgerror/errors.pb.go b/pkg/sql/pgwire/pgerror/errors.pb.go index d0a0bdf8cd6b..2bd99cdb524f 100644 --- a/pkg/sql/pgwire/pgerror/errors.pb.go +++ b/pkg/sql/pgwire/pgerror/errors.pb.go @@ -27,19 +27,20 @@ const _ = proto.GoGoProtoPackageIsVersion2 // please upgrade the proto package type Error struct { // standard pg error fields. This can be passed // over the pg wire protocol. - Code string `protobuf:"bytes,1,opt,name=code,proto3" json:"code,omitempty"` - Message string `protobuf:"bytes,2,opt,name=message,proto3" json:"message,omitempty"` - Detail string `protobuf:"bytes,3,opt,name=detail,proto3" json:"detail,omitempty"` - Hint string `protobuf:"bytes,4,opt,name=hint,proto3" json:"hint,omitempty"` - Severity string `protobuf:"bytes,8,opt,name=severity,proto3" json:"severity,omitempty"` - Source *Error_Source `protobuf:"bytes,5,opt,name=source,proto3" json:"source,omitempty"` + Code string `protobuf:"bytes,1,opt,name=code,proto3" json:"code,omitempty"` + Message string `protobuf:"bytes,2,opt,name=message,proto3" json:"message,omitempty"` + Detail string `protobuf:"bytes,3,opt,name=detail,proto3" json:"detail,omitempty"` + Hint string `protobuf:"bytes,4,opt,name=hint,proto3" json:"hint,omitempty"` + Severity string `protobuf:"bytes,8,opt,name=severity,proto3" json:"severity,omitempty"` + ConstraintName string `protobuf:"bytes,9,opt,name=constraint_name,json=constraintName,proto3" json:"constraint_name,omitempty"` + Source *Error_Source `protobuf:"bytes,5,opt,name=source,proto3" json:"source,omitempty"` } func (m *Error) Reset() { *m = Error{} } func (m *Error) String() string { return proto.CompactTextString(m) } func (*Error) ProtoMessage() {} func (*Error) Descriptor() ([]byte, []int) { - return fileDescriptor_errors_aa3d3323b1cab3b8, []int{0} + return fileDescriptor_errors_fc85771fb5477664, []int{0} } func (m *Error) XXX_Unmarshal(b []byte) error { return m.Unmarshal(b) @@ -74,7 +75,7 @@ func (m *Error_Source) Reset() { *m = Error_Source{} } func (m *Error_Source) String() string { return proto.CompactTextString(m) } func (*Error_Source) ProtoMessage() {} func (*Error_Source) Descriptor() ([]byte, []int) { - return fileDescriptor_errors_aa3d3323b1cab3b8, []int{0, 0} + return fileDescriptor_errors_fc85771fb5477664, []int{0, 0} } func (m *Error_Source) XXX_Unmarshal(b []byte) error { return m.Unmarshal(b) @@ -158,6 +159,12 @@ func (m *Error) MarshalTo(dAtA []byte) (int, error) { i = encodeVarintErrors(dAtA, i, uint64(len(m.Severity))) i += copy(dAtA[i:], m.Severity) } + if len(m.ConstraintName) > 0 { + dAtA[i] = 0x4a + i++ + i = encodeVarintErrors(dAtA, i, uint64(len(m.ConstraintName))) + i += copy(dAtA[i:], m.ConstraintName) + } return i, nil } @@ -235,6 +242,10 @@ func (m *Error) Size() (n int) { if l > 0 { n += 1 + l + sovErrors(uint64(l)) } + l = len(m.ConstraintName) + if l > 0 { + n += 1 + l + sovErrors(uint64(l)) + } return n } @@ -478,6 +489,35 @@ func (m *Error) Unmarshal(dAtA []byte) error { } m.Severity = string(dAtA[iNdEx:postIndex]) iNdEx = postIndex + case 9: + if wireType != 2 { + return fmt.Errorf("proto: wrong wireType = %d for field ConstraintName", wireType) + } + var stringLen uint64 + for shift := uint(0); ; shift += 7 { + if shift >= 64 { + return ErrIntOverflowErrors + } + if iNdEx >= l { + return io.ErrUnexpectedEOF + } + b := dAtA[iNdEx] + iNdEx++ + stringLen |= (uint64(b) & 0x7F) << shift + if b < 0x80 { + break + } + } + intStringLen := int(stringLen) + if intStringLen < 0 { + return ErrInvalidLengthErrors + } + postIndex := iNdEx + intStringLen + if postIndex > l { + return io.ErrUnexpectedEOF + } + m.ConstraintName = string(dAtA[iNdEx:postIndex]) + iNdEx = postIndex default: iNdEx = preIndex skippy, err := skipErrors(dAtA[iNdEx:]) @@ -732,27 +772,29 @@ var ( ) func init() { - proto.RegisterFile("sql/pgwire/pgerror/errors.proto", fileDescriptor_errors_aa3d3323b1cab3b8) + proto.RegisterFile("sql/pgwire/pgerror/errors.proto", fileDescriptor_errors_fc85771fb5477664) } -var fileDescriptor_errors_aa3d3323b1cab3b8 = []byte{ - // 278 bytes of a gzipped FileDescriptorProto - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x64, 0x90, 0xbf, 0x4e, 0xf3, 0x30, - 0x14, 0xc5, 0xe3, 0x7e, 0xf9, 0xe3, 0xcf, 0x2c, 0xc5, 0x03, 0xb2, 0x3a, 0xb8, 0x15, 0x53, 0x59, - 0x1c, 0x09, 0x06, 0x76, 0x24, 0x96, 0x8a, 0x29, 0x6c, 0x6c, 0xc1, 0x75, 0x53, 0x8b, 0x10, 0x07, - 0xdb, 0x05, 0xf1, 0x16, 0x7d, 0xac, 0x8e, 0x1d, 0x3b, 0x42, 0xf2, 0x22, 0xc8, 0x8e, 0xc9, 0xc2, - 0x62, 0x9d, 0x73, 0xef, 0xd1, 0xf1, 0x4f, 0x17, 0xcd, 0xcd, 0x5b, 0x9d, 0xb7, 0xd5, 0x87, 0xd4, - 0x22, 0x6f, 0x2b, 0xa1, 0xb5, 0xd2, 0xb9, 0x7f, 0x0d, 0x6b, 0xb5, 0xb2, 0x0a, 0x9f, 0x73, 0xc5, - 0x5f, 0xb4, 0x2a, 0xf9, 0x96, 0x85, 0xfd, 0xe5, 0x7e, 0x82, 0x92, 0x7b, 0xa7, 0x30, 0x46, 0x31, - 0x57, 0x6b, 0x41, 0xc0, 0x02, 0x2c, 0xff, 0x17, 0x5e, 0x63, 0x82, 0xb2, 0x57, 0x61, 0x4c, 0x59, - 0x09, 0x32, 0xf1, 0xe3, 0x5f, 0x8b, 0x2f, 0x50, 0xba, 0x16, 0xb6, 0x94, 0x35, 0xf9, 0xe7, 0x17, - 0xc1, 0xb9, 0x96, 0xad, 0x6c, 0x2c, 0x89, 0x87, 0x16, 0xa7, 0xf1, 0x2d, 0x4a, 0x8d, 0xda, 0x69, - 0x2e, 0x48, 0xb2, 0x00, 0xcb, 0xb3, 0xeb, 0x39, 0xfb, 0xc3, 0xc1, 0x3c, 0x03, 0x7b, 0xf4, 0xb1, - 0x22, 0xc4, 0xf1, 0x0c, 0x41, 0x23, 0xde, 0x85, 0x96, 0xf6, 0x93, 0x40, 0x5f, 0x38, 0xfa, 0xd9, - 0x03, 0x4a, 0x87, 0xb4, 0xfb, 0x72, 0x23, 0xeb, 0x11, 0xdc, 0x69, 0x37, 0xab, 0x65, 0x33, 0x50, - 0x27, 0x85, 0xd7, 0xae, 0x6d, 0xb3, 0x6b, 0xb8, 0x95, 0xaa, 0x09, 0xd0, 0xa3, 0x5f, 0xc5, 0x30, - 0x9d, 0x66, 0xab, 0x18, 0x66, 0x53, 0x78, 0x77, 0x75, 0xf8, 0xa6, 0xd1, 0xa1, 0xa3, 0xe0, 0xd8, - 0x51, 0x70, 0xea, 0x28, 0xf8, 0xea, 0x28, 0xd8, 0xf7, 0x34, 0x3a, 0xf6, 0x34, 0x3a, 0xf5, 0x34, - 0x7a, 0xca, 0x02, 0xf5, 0x73, 0xea, 0xef, 0x7a, 0xf3, 0x13, 0x00, 0x00, 0xff, 0xff, 0x88, 0x15, - 0xdc, 0x81, 0x7a, 0x01, 0x00, 0x00, +var fileDescriptor_errors_fc85771fb5477664 = []byte{ + // 306 bytes of a gzipped FileDescriptorProto + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x64, 0x51, 0xb1, 0x4e, 0xf3, 0x30, + 0x18, 0x8c, 0xfb, 0xa7, 0x49, 0xea, 0x5f, 0x82, 0xe2, 0x01, 0x59, 0x1d, 0xdc, 0x8a, 0x85, 0xb2, + 0xa4, 0x12, 0x0c, 0xec, 0x48, 0x2c, 0x15, 0x62, 0x28, 0x1b, 0x0b, 0x32, 0xae, 0x9b, 0x5a, 0x24, + 0x76, 0xb0, 0x5d, 0x10, 0x6f, 0xc1, 0xeb, 0xf0, 0x06, 0x1d, 0x3b, 0x76, 0x84, 0xe4, 0x45, 0x90, + 0x9d, 0x10, 0x06, 0x96, 0xe8, 0xee, 0xbe, 0xd3, 0xe5, 0x4e, 0x86, 0x63, 0xf3, 0x9c, 0xcf, 0xca, + 0xec, 0x55, 0x68, 0x3e, 0x2b, 0x33, 0xae, 0xb5, 0xd2, 0x33, 0xff, 0x35, 0x69, 0xa9, 0x95, 0x55, + 0xe8, 0x88, 0x29, 0xf6, 0xa4, 0x15, 0x65, 0xeb, 0xb4, 0xbd, 0x9f, 0x7c, 0xf4, 0x60, 0xff, 0xda, + 0x21, 0x84, 0x60, 0xc8, 0xd4, 0x92, 0x63, 0x30, 0x01, 0xd3, 0xc1, 0xc2, 0x63, 0x84, 0x61, 0x5c, + 0x70, 0x63, 0x68, 0xc6, 0x71, 0xcf, 0xcb, 0x3f, 0x14, 0x1d, 0xc3, 0x68, 0xc9, 0x2d, 0x15, 0x39, + 0xfe, 0xe7, 0x0f, 0x2d, 0x73, 0x29, 0x6b, 0x21, 0x2d, 0x0e, 0x9b, 0x14, 0x87, 0xd1, 0x25, 0x8c, + 0x8c, 0xda, 0x68, 0xc6, 0x71, 0x7f, 0x02, 0xa6, 0xff, 0xcf, 0xc7, 0xe9, 0x9f, 0x1e, 0xa9, 0xef, + 0x90, 0xde, 0x79, 0xdb, 0xa2, 0xb5, 0xa3, 0x11, 0x4c, 0x0c, 0x7f, 0xe1, 0x5a, 0xd8, 0x37, 0x9c, + 0xf8, 0xc0, 0x8e, 0xa3, 0x53, 0x78, 0xc8, 0x94, 0x34, 0x56, 0x53, 0x21, 0xed, 0x83, 0xa4, 0x05, + 0xc7, 0x03, 0x6f, 0x39, 0xf8, 0x95, 0x6f, 0x69, 0xc1, 0x47, 0x37, 0x30, 0x6a, 0x62, 0x5d, 0xb7, + 0x95, 0xc8, 0xbb, 0x85, 0x0e, 0x3b, 0x2d, 0x17, 0xb2, 0x99, 0xd7, 0x5f, 0x78, 0xec, 0x7e, 0xbb, + 0xda, 0x48, 0x66, 0x85, 0x92, 0xed, 0xba, 0x8e, 0xcf, 0xc3, 0x24, 0x1a, 0xc6, 0xf3, 0x30, 0x89, + 0x87, 0xc9, 0xd5, 0xd9, 0xf6, 0x8b, 0x04, 0xdb, 0x8a, 0x80, 0x5d, 0x45, 0xc0, 0xbe, 0x22, 0xe0, + 0xb3, 0x22, 0xe0, 0xbd, 0x26, 0xc1, 0xae, 0x26, 0xc1, 0xbe, 0x26, 0xc1, 0x7d, 0xdc, 0xce, 0x7b, + 0x8c, 0xfc, 0x03, 0x5c, 0x7c, 0x07, 0x00, 0x00, 0xff, 0xff, 0xc9, 0xa7, 0x8a, 0xfb, 0xa3, 0x01, + 0x00, 0x00, } diff --git a/pkg/sql/pgwire/pgerror/errors.proto b/pkg/sql/pgwire/pgerror/errors.proto index fb73659bdf27..ceb515b120f9 100644 --- a/pkg/sql/pgwire/pgerror/errors.proto +++ b/pkg/sql/pgwire/pgerror/errors.proto @@ -24,6 +24,7 @@ message Error { string detail = 3; string hint = 4; string severity = 8; + string constraint_name = 9; message Source { string file = 1; diff --git a/pkg/sql/pgwire/pgerror/flatten.go b/pkg/sql/pgwire/pgerror/flatten.go index 6f321d0a135b..8b209bf10754 100644 --- a/pkg/sql/pgwire/pgerror/flatten.go +++ b/pkg/sql/pgwire/pgerror/flatten.go @@ -37,9 +37,10 @@ func Flatten(err error) *Error { return nil } resErr := &Error{ - Code: GetPGCode(err).String(), - Message: err.Error(), - Severity: GetSeverity(err), + Code: GetPGCode(err).String(), + Message: err.Error(), + Severity: GetSeverity(err), + ConstraintName: GetConstraintName(err), } // Populate the source field if available. diff --git a/pkg/sql/pgwire/pgwirebase/msg.go b/pkg/sql/pgwire/pgwirebase/msg.go index c21a2959e64f..0eb8f478c80a 100644 --- a/pkg/sql/pgwire/pgwirebase/msg.go +++ b/pkg/sql/pgwire/pgwirebase/msg.go @@ -60,14 +60,15 @@ type ServerErrFieldType byte // http://www.postgresql.org/docs/current/static/protocol-error-fields.html const ( - ServerErrFieldSeverity ServerErrFieldType = 'S' - ServerErrFieldSQLState ServerErrFieldType = 'C' - ServerErrFieldMsgPrimary ServerErrFieldType = 'M' - ServerErrFileldDetail ServerErrFieldType = 'D' - ServerErrFileldHint ServerErrFieldType = 'H' - ServerErrFieldSrcFile ServerErrFieldType = 'F' - ServerErrFieldSrcLine ServerErrFieldType = 'L' - ServerErrFieldSrcFunction ServerErrFieldType = 'R' + ServerErrFieldSeverity ServerErrFieldType = 'S' + ServerErrFieldSQLState ServerErrFieldType = 'C' + ServerErrFieldMsgPrimary ServerErrFieldType = 'M' + ServerErrFieldDetail ServerErrFieldType = 'D' + ServerErrFieldHint ServerErrFieldType = 'H' + ServerErrFieldSrcFile ServerErrFieldType = 'F' + ServerErrFieldSrcLine ServerErrFieldType = 'L' + ServerErrFieldSrcFunction ServerErrFieldType = 'R' + ServerErrFieldConstraintName ServerErrFieldType = 'n' ) // PrepareType represents a subtype for prepare messages. diff --git a/pkg/sql/pgwire/pgwirebase/servererrfieldtype_string.go b/pkg/sql/pgwire/pgwirebase/servererrfieldtype_string.go index 629bae8f4019..281aab78cb8e 100644 --- a/pkg/sql/pgwire/pgwirebase/servererrfieldtype_string.go +++ b/pkg/sql/pgwire/pgwirebase/servererrfieldtype_string.go @@ -11,23 +11,25 @@ func _() { _ = x[ServerErrFieldSeverity-83] _ = x[ServerErrFieldSQLState-67] _ = x[ServerErrFieldMsgPrimary-77] - _ = x[ServerErrFileldDetail-68] - _ = x[ServerErrFileldHint-72] + _ = x[ServerErrFieldDetail-68] + _ = x[ServerErrFieldHint-72] _ = x[ServerErrFieldSrcFile-70] _ = x[ServerErrFieldSrcLine-76] _ = x[ServerErrFieldSrcFunction-82] + _ = x[ServerErrFieldConstraintName-110] } const ( - _ServerErrFieldType_name_0 = "ServerErrFieldSQLStateServerErrFileldDetail" + _ServerErrFieldType_name_0 = "ServerErrFieldSQLStateServerErrFieldDetail" _ServerErrFieldType_name_1 = "ServerErrFieldSrcFile" - _ServerErrFieldType_name_2 = "ServerErrFileldHint" + _ServerErrFieldType_name_2 = "ServerErrFieldHint" _ServerErrFieldType_name_3 = "ServerErrFieldSrcLineServerErrFieldMsgPrimary" _ServerErrFieldType_name_4 = "ServerErrFieldSrcFunctionServerErrFieldSeverity" + _ServerErrFieldType_name_5 = "ServerErrFieldConstraintName" ) var ( - _ServerErrFieldType_index_0 = [...]uint8{0, 22, 43} + _ServerErrFieldType_index_0 = [...]uint8{0, 22, 42} _ServerErrFieldType_index_3 = [...]uint8{0, 21, 45} _ServerErrFieldType_index_4 = [...]uint8{0, 25, 47} ) @@ -47,6 +49,8 @@ func (i ServerErrFieldType) String() string { case 82 <= i && i <= 83: i -= 82 return _ServerErrFieldType_name_4[_ServerErrFieldType_index_4[i]:_ServerErrFieldType_index_4[i+1]] + case i == 110: + return _ServerErrFieldType_name_5 default: return "ServerErrFieldType(" + strconv.FormatInt(int64(i), 10) + ")" } diff --git a/pkg/sql/pgwire/testdata/pgtest/errors b/pkg/sql/pgwire/testdata/pgtest/errors new file mode 100644 index 000000000000..d5041e3f9198 --- /dev/null +++ b/pkg/sql/pgwire/testdata/pgtest/errors @@ -0,0 +1,76 @@ +# This test verifies that we're populating the Constraint field of an error +# generated by a foreign key constraint violation. + +# Prepare the environment. +send +Query {"String": "DROP TABLE IF EXISTS child, parent"} +---- + +until ignore=NoticeResponse +ReadyForQuery +---- +{"Type":"CommandComplete","CommandTag":"DROP TABLE"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +send +Query {"String": "CREATE TABLE parent (p INT PRIMARY KEY)"} +---- + +until +ReadyForQuery +---- +{"Type":"CommandComplete","CommandTag":"CREATE TABLE"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +send +Query {"String": "CREATE TABLE child (c INT PRIMARY KEY, p INT REFERENCES parent(p))"} +---- + +until +ReadyForQuery +---- +{"Type":"CommandComplete","CommandTag":"CREATE TABLE"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +send +Query {"String": "INSERT INTO child VALUES (1,1)"} +---- + +until +ErrorResponse +ReadyForQuery +---- +{"Type":"ErrorResponse","Code":"23503","ConstraintName":"fk_p_ref_parent"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +send +Query {"String": "ALTER TABLE child DROP CONSTRAINT fk_p_ref_parent"} +---- + +# Test with a constraint name that requires quoting. +until +ReadyForQuery +---- +{"Type":"CommandComplete","CommandTag":"ALTER TABLE"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +send +Query {"String": "ALTER TABLE child ADD CONSTRAINT \"foo bar\" FOREIGN KEY (p) REFERENCES parent(p)"} +---- + +until +ReadyForQuery +---- +{"Type":"CommandComplete","CommandTag":"ALTER TABLE"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +send +Query {"String": "INSERT INTO child VALUES (1,1)"} +---- + +until +ErrorResponse +ReadyForQuery +---- +{"Type":"ErrorResponse","Code":"23503","ConstraintName":"foo bar"} +{"Type":"ReadyForQuery","TxStatus":"I"} diff --git a/pkg/testutils/lint/lint_test.go b/pkg/testutils/lint/lint_test.go index 7223ace3f82d..1570a39d22a7 100644 --- a/pkg/testutils/lint/lint_test.go +++ b/pkg/testutils/lint/lint_test.go @@ -1020,6 +1020,7 @@ func TestLint(t *testing.T) { "*.go", ":!*.pb.go", ":!*.pb.gw.go", + ":!sql/pgwire/pgerror/constraint_name.go", ":!sql/pgwire/pgerror/severity.go", ":!sql/pgwire/pgerror/with_candidate_code.go", ":!sql/pgwire/pgwirebase/too_big_error.go", @@ -1843,6 +1844,8 @@ func TestLint(t *testing.T) { // error encode/decode, it can be dropped from the linter // exception as well. stream.GrepNot(`pkg/roachpb/errors\.go:.*invalid direct cast on error object`), + // Cast in decode handler. + stream.GrepNot(`pkg/sql/pgwire/pgerror/constraint_name\.go:.*invalid direct cast on error object`), // pgerror's pgcode logic uses its own custom cause recursion // algorithm and thus cannot use errors.If() which mandates a // different recursion order. diff --git a/pkg/testutils/pgtest/datadriven.go b/pkg/testutils/pgtest/datadriven.go index ec25880cae36..bd9c733b0e41 100644 --- a/pkg/testutils/pgtest/datadriven.go +++ b/pkg/testutils/pgtest/datadriven.go @@ -204,13 +204,15 @@ func msgsToJSONWithIgnore(msgs []pgproto3.BackendMessage, args *datadriven.TestD code = v } if err := enc.Encode(struct { - Type string - Code string - Message string `json:",omitempty"` + Type string + Code string + Message string `json:",omitempty"` + ConstraintName string `json:",omitempty"` }{ - Type: "ErrorResponse", - Code: code, - Message: errmsg.Message, + Type: "ErrorResponse", + Code: code, + Message: errmsg.Message, + ConstraintName: errmsg.ConstraintName, }); err != nil { panic(err) } diff --git a/pkg/testutils/pgtest/pgtest.go b/pkg/testutils/pgtest/pgtest.go index 93d181064eb0..7f3c23bed893 100644 --- a/pkg/testutils/pgtest/pgtest.go +++ b/pkg/testutils/pgtest/pgtest.go @@ -134,8 +134,9 @@ func (p *PGTest) Until( // ErrorResponse doesn't encode/decode correctly, so // manually append it here. msgs = append(msgs, &pgproto3.ErrorResponse{ - Code: errmsg.Code, - Message: message, + Code: errmsg.Code, + Message: message, + ConstraintName: errmsg.ConstraintName, }) typs = typs[1:] continue From c47d2934243f496d07d7eb4e037fe448ad1818d4 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 17 Sep 2020 16:42:36 -0700 Subject: [PATCH 2/2] sql: enable partial inverted indexes in randomized tests Now that partial inverted indexes can be created, they can be generated in randomized tests. Release note: None --- pkg/sql/rowenc/testutils.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/pkg/sql/rowenc/testutils.go b/pkg/sql/rowenc/testutils.go index 1cabd0f71fe6..e5084b4379e9 100644 --- a/pkg/sql/rowenc/testutils.go +++ b/pkg/sql/rowenc/testutils.go @@ -1390,12 +1390,6 @@ func PartialIndexMutator(rng *rand.Rand, stmts []tree.Statement) ([]tree.Stateme for _, stmt := range stmts { switch ast := stmt.(type) { case *tree.CreateIndex: - // TODO(mgartner): Create partial inverted indexes when they are - // fully supported. - if ast.Inverted { - continue - } - tableInfo, ok := tables[ast.Table.ObjectName] if !ok { continue @@ -1424,9 +1418,7 @@ func PartialIndexMutator(rng *rand.Rand, stmts []tree.Statement) ([]tree.Stateme } } - // TODO(mgartner): Create partial inverted indexes when they are - // fully supported. - if idx == nil || idx.Inverted { + if idx == nil { continue }