Add feature flag to remove use of "INSERT RETURNING" in NewOrderAndAu…

…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
letsencrypt · Oct 4, 2024 · 7b032a6 · 7b032a6
1 parent 58f515e
commit 7b032a6
Show file tree

Hide file tree

Showing 4 changed files with 109 additions and 25 deletions.
diff --git a/features/features.go b/features/features.go
@@ -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)

diff --git a/sa/sa.go b/sa/sa.go
@@ -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
+				}
 			}
 		}
 

diff --git a/sa/sa_test.go b/sa/sa_test.go
@@ -9,6 +9,7 @@ import (
 	"crypto/x509"
 	"database/sql"
 	"encoding/base64"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -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()
 
@@ -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),
@@ -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
@@ -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)
@@ -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,
@@ -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"
@@ -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()

diff --git a/test/config-next/sa.json b/test/config-next/sa.json
@@ -51,7 +51,8 @@
 		"features": {
 			"MultipleCertificateProfiles": true,
 			"TrackReplacementCertificatesARI": true,
-			"DisableLegacyLimitWrites": true
+			"DisableLegacyLimitWrites": true,
+			"InsertAuthzsIndividually": true
 		}
 	},
 	"syslog": {