Skip to content

Commit

Permalink
Add feature flag to remove use of "INSERT RETURNING" in NewOrderAndAu…
Browse files Browse the repository at this point in the history
…thzs (#7739)

This is our only use of MariaDB's "INSERT ... RETURNING" syntax, which
does not exist in MySQL and Vitess. Add a feature flag which removes our
use of this feature, so that we can easily disable it and then re-enable
it if it turns out to be too much of a performance hit.

Also add a benchmark showing that the serial-insertion approach is
slower, but perhaps not debilitatingly so.

Part of #7718
  • Loading branch information
aarongable authored Oct 4, 2024
1 parent 58f515e commit 7b032a6
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 25 deletions.
7 changes: 7 additions & 0 deletions features/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,13 @@ type Config struct {
//
// This flag should only be used in conjunction with UseKvLimitsForNewOrder.
DisableLegacyLimitWrites bool

// InsertAuthzsIndividually causes the SA's NewOrderAndAuthzs method to
// create each new authz one at a time, rather than using MultiInserter.
// Although this is expected to be a performance penalty, it is necessary to
// get the AUTO_INCREMENT ID of each new authz without relying on MariaDB's
// unique "INSERT ... RETURNING" functionality.
InsertAuthzsIndividually bool
}

var fMu = new(sync.RWMutex)
Expand Down
58 changes: 36 additions & 22 deletions sa/sa.go
Original file line number Diff line number Diff line change
Expand Up @@ -474,37 +474,51 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
output, err := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) {
// First, insert all of the new authorizations and record their IDs.
newAuthzIDs := make([]int64, 0)
if len(req.NewAuthzs) != 0 {
inserter, err := db.NewMultiInserter("authz2", strings.Split(authzFields, ", "), "id")
if err != nil {
return nil, err
}
if features.Get().InsertAuthzsIndividually {
for _, authz := range req.NewAuthzs {
am, err := newAuthzReqToModel(authz)
if err != nil {
return nil, err
}
err = inserter.Add([]interface{}{
am.ID,
am.IdentifierType,
am.IdentifierValue,
am.RegistrationID,
statusToUint[core.StatusPending],
am.Expires,
am.Challenges,
nil,
nil,
am.Token,
nil,
nil,
})
err = tx.Insert(ctx, am)
if err != nil {
return nil, err
}
newAuthzIDs = append(newAuthzIDs, am.ID)
}
newAuthzIDs, err = inserter.Insert(ctx, tx)
if err != nil {
return nil, err
} else {
if len(req.NewAuthzs) != 0 {
inserter, err := db.NewMultiInserter("authz2", strings.Split(authzFields, ", "), "id")
if err != nil {
return nil, err
}
for _, authz := range req.NewAuthzs {
am, err := newAuthzReqToModel(authz)
if err != nil {
return nil, err
}
err = inserter.Add([]interface{}{
am.ID,
am.IdentifierType,
am.IdentifierValue,
am.RegistrationID,
statusToUint[core.StatusPending],
am.Expires,
am.Challenges,
nil,
nil,
am.Token,
nil,
nil,
})
if err != nil {
return nil, err
}
}
newAuthzIDs, err = inserter.Insert(ctx, tx)
if err != nil {
return nil, err
}
}
}

Expand Down
66 changes: 64 additions & 2 deletions sa/sa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"crypto/x509"
"database/sql"
"encoding/base64"
"encoding/hex"
"encoding/json"
"errors"
"fmt"
Expand Down Expand Up @@ -77,7 +78,7 @@ func (s *fakeServerStream[T]) Context() context.Context {

// initSA constructs a SQLStorageAuthority and a clean up function that should
// be defer'ed to the end of the test.
func initSA(t *testing.T) (*SQLStorageAuthority, clock.FakeClock, func()) {
func initSA(t testing.TB) (*SQLStorageAuthority, clock.FakeClock, func()) {
t.Helper()
features.Reset()

Expand Down Expand Up @@ -109,7 +110,7 @@ func initSA(t *testing.T) (*SQLStorageAuthority, clock.FakeClock, func()) {

// CreateWorkingTestRegistration inserts a new, correct Registration into the
// given SA.
func createWorkingRegistration(t *testing.T, sa *SQLStorageAuthority) *corepb.Registration {
func createWorkingRegistration(t testing.TB, sa *SQLStorageAuthority) *corepb.Registration {
initialIP, _ := net.ParseIP("88.77.66.11").MarshalText()
reg, err := sa.NewRegistration(context.Background(), &corepb.Registration{
Key: []byte(theKey),
Expand Down Expand Up @@ -1231,6 +1232,9 @@ func TestNewOrderAndAuthzs(t *testing.T) {
sa, _, cleanup := initSA(t)
defer cleanup()

features.Set(features.Config{InsertAuthzsIndividually: true})
defer features.Reset()

reg := createWorkingRegistration(t, sa)

// Insert two pre-existing authorizations to reference
Expand Down Expand Up @@ -1285,6 +1289,9 @@ func TestNewOrderAndAuthzs_NonNilInnerOrder(t *testing.T) {
sa, fc, cleanup := initSA(t)
defer cleanup()

features.Set(features.Config{InsertAuthzsIndividually: true})
defer features.Reset()

reg := createWorkingRegistration(t, sa)

expires := fc.Now().Add(2 * time.Hour)
Expand All @@ -1306,6 +1313,9 @@ func TestNewOrderAndAuthzs_MismatchedRegID(t *testing.T) {
sa, _, cleanup := initSA(t)
defer cleanup()

features.Set(features.Config{InsertAuthzsIndividually: true})
defer features.Reset()

_, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
NewOrder: &sapb.NewOrderRequest{
RegistrationID: 1,
Expand All @@ -1324,6 +1334,9 @@ func TestNewOrderAndAuthzs_NewAuthzExpectedFields(t *testing.T) {
sa, fc, cleanup := initSA(t)
defer cleanup()

features.Set(features.Config{InsertAuthzsIndividually: true})
defer features.Reset()

reg := createWorkingRegistration(t, sa)
expires := fc.Now().Add(time.Hour)
domain := "a.com"
Expand Down Expand Up @@ -1372,6 +1385,55 @@ func TestNewOrderAndAuthzs_NewAuthzExpectedFields(t *testing.T) {
test.AssertBoxedNil(t, am.ValidationRecord, "am.ValidationRecord should be nil")
}

func BenchmarkNewOrderAndAuthzs(b *testing.B) {
for _, flag := range []bool{false, true} {
for _, numIdents := range []int{1, 2, 5, 10, 20, 50, 100} {
b.Run(fmt.Sprintf("%t/%d", flag, numIdents), func(b *testing.B) {
sa, _, cleanup := initSA(b)
defer cleanup()

if flag {
features.Set(features.Config{InsertAuthzsIndividually: true})
defer features.Reset()
}

reg := createWorkingRegistration(b, sa)

dnsNames := make([]string, 0, numIdents)
newAuthzs := make([]*sapb.NewAuthzRequest, 0, numIdents)
for range numIdents {
var nameBytes [3]byte
_, _ = rand.Read(nameBytes[:])
name := fmt.Sprintf("%s.example.com", hex.EncodeToString(nameBytes[:]))

dnsNames = append(dnsNames, name)
newAuthzs = append(newAuthzs, &sapb.NewAuthzRequest{
RegistrationID: reg.Id,
Identifier: identifier.NewDNS(name).AsProto(),
ChallengeTypes: []string{string(core.ChallengeTypeDNS01)},
Token: core.NewToken(),
Expires: timestamppb.New(sa.clk.Now().Add(24 * time.Hour)),
})
}

b.ResetTimer()

_, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
NewOrder: &sapb.NewOrderRequest{
RegistrationID: reg.Id,
Expires: timestamppb.New(sa.clk.Now().Add(24 * time.Hour)),
DnsNames: dnsNames,
},
NewAuthzs: newAuthzs,
})
if err != nil {
b.Error(err)
}
})
}
}
}

func TestSetOrderProcessing(t *testing.T) {
sa, fc, cleanup := initSA(t)
defer cleanup()
Expand Down
3 changes: 2 additions & 1 deletion test/config-next/sa.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@
"features": {
"MultipleCertificateProfiles": true,
"TrackReplacementCertificatesARI": true,
"DisableLegacyLimitWrites": true
"DisableLegacyLimitWrites": true,
"InsertAuthzsIndividually": true
}
},
"syslog": {
Expand Down

0 comments on commit 7b032a6

Please sign in to comment.