Remove 'RETURNING' functionality from MultiInserter (#7740)

Deprecate the "InsertAuthzsIndividually" feature flag, which has been
set to true in both Staging and Production. Delete the code guarded
behind that flag being false, namely the ability of the MultiInserter to
return the newly-created IDs from all of the rows it has inserted. This
behavior is being removed because it is not supported in MySQL / Vitess.

Fixes https://github.com/letsencrypt/boulder/issues/7718

---

> [!WARNING]
> ~~Do not merge until IN-10737 is complete~~

db/interfaces.go

@@ -58,17 +58,9 @@ type Executor interface {
 	OneSelector
 	Inserter
 	SelectExecer
-	Queryer
 	Delete(context.Context, ...interface{}) (int64, error)
 	Get(context.Context, interface{}, ...interface{}) (interface{}, error)
 	Update(context.Context, ...interface{}) (int64, error)
-}
-
-// Queryer offers the QueryContext method. Note that this is not read-only (i.e. not
-// Selector), since a QueryContext can be `INSERT`, `UPDATE`, etc. The difference
-// between QueryContext and ExecContext is that QueryContext can return rows. So for instance it is
-// suitable for inserting rows and getting back ids.
-type Queryer interface {
 	QueryContext(context.Context, string, ...interface{}) (*sql.Rows, error)
 }
 

db/multi.go

@@ -7,7 +7,7 @@ import (
 )
 
 // MultiInserter makes it easy to construct a
-// `INSERT INTO table (...) VALUES ... RETURNING id;`
+// `INSERT INTO table (...) VALUES ...;`
 // query which inserts multiple rows into the same table. It can also execute
 // the resulting query.
 type MultiInserter struct {
@@ -16,20 +16,15 @@ type MultiInserter struct {
 	// https://mariadb.com/kb/en/identifier-names/#unquoted
 	table  string
 	fields []string
-	returningColumn string
 
 	values [][]interface{}
 }
 
 // NewMultiInserter creates a new MultiInserter, checking for reasonable table
-// name and list of fields. returningColumn is the name of a column to be used
+// name and list of fields.
-// in a `RETURNING xyz` clause at the end. If it is empty, no `RETURNING xyz`
+// Safety: `table` and `fields` must contain only strings that are known at
-// clause is used. If returningColumn is present, it must refer to a column
+// compile time. They must not contain user-controlled strings.
-// that can be parsed into an int64.
+func NewMultiInserter(table string, fields []string) (*MultiInserter, error) {
-// Safety: `table`, `fields`, and `returningColumn` must contain only strings
-// that are known at compile time. They must not contain user-controlled
-// strings.
-func NewMultiInserter(table string, fields []string, returningColumn string) (*MultiInserter, error) {
 	if len(table) == 0 || len(fields) == 0 {
 		return nil, fmt.Errorf("empty table name or fields list")
 	}
@@ -44,17 +39,10 @@ func NewMultiInserter(table string, fields []string, returningColumn string) (*M
 			return nil, err
 		}
 	}
-	if returningColumn != "" {
-		err := validMariaDBUnquotedIdentifier(returningColumn)
-		if err != nil {
-			return nil, err
-		}
-	}
 
 	return &MultiInserter{
 		table:  table,
 		fields: fields,
-		returningColumn: returningColumn,
 		values: make([][]interface{}, 0),
 	}, nil
 }
@@ -84,56 +72,32 @@ func (mi *MultiInserter) query() (string, []interface{}) {
 
 	questions := strings.TrimRight(questionsBuf.String(), ",")
 
-	// Safety: we are interpolating `mi.returningColumn` into an SQL query. We
-	// know it is a valid unquoted identifier in MariaDB because we verified
-	// that in the constructor.
-	returning := ""
-	if mi.returningColumn != "" {
-		returning = fmt.Sprintf(" RETURNING %s", mi.returningColumn)
-	}
 	// Safety: we are interpolating `mi.table` and `mi.fields` into an SQL
 	// query. We know they contain, respectively, a valid unquoted identifier
 	// and a slice of valid unquoted identifiers because we verified that in
 	// the constructor. We know the query overall has valid syntax because we
 	// generate it entirely within this function.
-	query := fmt.Sprintf("INSERT INTO %s (%s) VALUES %s%s", mi.table, strings.Join(mi.fields, ","), questions, returning)
+	query := fmt.Sprintf("INSERT INTO %s (%s) VALUES %s", mi.table, strings.Join(mi.fields, ","), questions)
 
 	return query, queryArgs
 }
 
 // Insert inserts all the collected rows into the database represented by
-// `queryer`. If a non-empty returningColumn was provided, then it returns
+// `queryer`.
-// the list of values from that column returned by the query.
+func (mi *MultiInserter) Insert(ctx context.Context, db Execer) error {
-func (mi *MultiInserter) Insert(ctx context.Context, queryer Queryer) ([]int64, error) {
 	query, queryArgs := mi.query()
-	rows, err := queryer.QueryContext(ctx, query, queryArgs...)
+	res, err := db.ExecContext(ctx, query, queryArgs...)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	ids := make([]int64, 0, len(mi.values))
+	affected, err := res.RowsAffected()
-	if mi.returningColumn != "" {
-		for rows.Next() {
-			var id int64
-			err = rows.Scan(&id)
 	if err != nil {
-				rows.Close()
+		return err
-				return nil, err
-			}
-			ids = append(ids, id)
 	}
+	if affected != int64(len(mi.values)) {
+		return fmt.Errorf("unexpected number of rows inserted: %d != %d", affected, len(mi.values))
 	}
 
-	// Hack: sometimes in unittests we make a mock Queryer that returns a nil
+	return nil
-	// `*sql.Rows`. A nil `*sql.Rows` is not actually valid— calling `Close()`
-	// on it will panic— but here we choose to treat it like an empty list,
-	// and skip calling `Close()` to avoid the panic.
-	if rows != nil {
-		err = rows.Close()
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	return ids, nil
 }

db/multi_test.go

@@ -7,34 +7,29 @@ import (
 )
 
 func TestNewMulti(t *testing.T) {
-	_, err := NewMultiInserter("", []string{"colA"}, "")
+	_, err := NewMultiInserter("", []string{"colA"})
 	test.AssertError(t, err, "Empty table name should fail")
 
-	_, err = NewMultiInserter("myTable", nil, "")
+	_, err = NewMultiInserter("myTable", nil)
 	test.AssertError(t, err, "Empty fields list should fail")
 
-	mi, err := NewMultiInserter("myTable", []string{"colA"}, "")
+	mi, err := NewMultiInserter("myTable", []string{"colA"})
 	test.AssertNotError(t, err, "Single-column construction should not fail")
 	test.AssertEquals(t, len(mi.fields), 1)
 
-	mi, err = NewMultiInserter("myTable", []string{"colA", "colB", "colC"}, "")
+	mi, err = NewMultiInserter("myTable", []string{"colA", "colB", "colC"})
 	test.AssertNotError(t, err, "Multi-column construction should not fail")
 	test.AssertEquals(t, len(mi.fields), 3)
 
-	_, err = NewMultiInserter("", []string{"colA"}, "colB")
+	_, err = NewMultiInserter("foo\"bar", []string{"colA"})
-	test.AssertError(t, err, "expected error for empty table name")
-	_, err = NewMultiInserter("foo\"bar", []string{"colA"}, "colB")
 	test.AssertError(t, err, "expected error for invalid table name")
 
-	_, err = NewMultiInserter("myTable", []string{"colA", "foo\"bar"}, "colB")
+	_, err = NewMultiInserter("myTable", []string{"colA", "foo\"bar"})
 	test.AssertError(t, err, "expected error for invalid column name")
-
-	_, err = NewMultiInserter("myTable", []string{"colA"}, "foo\"bar")
-	test.AssertError(t, err, "expected error for invalid returning column name")
 }
 
 func TestMultiAdd(t *testing.T) {
-	mi, err := NewMultiInserter("table", []string{"a", "b", "c"}, "")
+	mi, err := NewMultiInserter("table", []string{"a", "b", "c"})
 	test.AssertNotError(t, err, "Failed to create test MultiInserter")
 
 	err = mi.Add([]interface{}{})
@@ -57,7 +52,7 @@ func TestMultiAdd(t *testing.T) {
 }
 
 func TestMultiQuery(t *testing.T) {
-	mi, err := NewMultiInserter("table", []string{"a", "b", "c"}, "")
+	mi, err := NewMultiInserter("table", []string{"a", "b", "c"})
 	test.AssertNotError(t, err, "Failed to create test MultiInserter")
 	err = mi.Add([]interface{}{"one", "two", "three"})
 	test.AssertNotError(t, err, "Failed to insert test row")
@@ -67,15 +62,4 @@ func TestMultiQuery(t *testing.T) {
 	query, queryArgs := mi.query()
 	test.AssertEquals(t, query, "INSERT INTO table (a,b,c) VALUES (?,?,?),(?,?,?)")
 	test.AssertDeepEquals(t, queryArgs, []interface{}{"one", "two", "three", "egy", "kettö", "három"})
-
-	mi, err = NewMultiInserter("table", []string{"a", "b", "c"}, "id")
-	test.AssertNotError(t, err, "Failed to create test MultiInserter")
-	err = mi.Add([]interface{}{"one", "two", "three"})
-	test.AssertNotError(t, err, "Failed to insert test row")
-	err = mi.Add([]interface{}{"egy", "kettö", "három"})
-	test.AssertNotError(t, err, "Failed to insert test row")
-
-	query, queryArgs = mi.query()
-	test.AssertEquals(t, query, "INSERT INTO table (a,b,c) VALUES (?,?,?),(?,?,?) RETURNING id")
-	test.AssertDeepEquals(t, queryArgs, []interface{}{"one", "two", "three", "egy", "kettö", "három"})
 }

features/features.go

@@ -20,6 +20,7 @@ type Config struct {
 	UseKvLimitsForNewOrder      bool
 	DisableLegacyLimitWrites    bool
 	MultipleCertificateProfiles bool
+	InsertAuthzsIndividually    bool
 
 	// ServeRenewalInfo exposes the renewalInfo endpoint in the directory and for
 	// GET requests. WARNING: This feature is a draft and highly unstable.
@@ -71,13 +72,6 @@ type Config struct {
 	// queries waiting for an available connection may be cancelled.
 	PropagateCancels 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
-
 	// AutomaticallyPauseZombieClients configures the RA to automatically track
 	// and pause issuance for each (account, hostname) pair that repeatedly
 	// fails validation.

sa/model.go

@@ -988,12 +988,12 @@ func deleteOrderFQDNSet(
 	return nil
 }
 
-func addIssuedNames(ctx context.Context, queryer db.Queryer, cert *x509.Certificate, isRenewal bool) error {
+func addIssuedNames(ctx context.Context, queryer db.Execer, cert *x509.Certificate, isRenewal bool) error {
 	if len(cert.DNSNames) == 0 {
 		return berrors.InternalServerError("certificate has no DNSNames")
 	}
 
-	multiInserter, err := db.NewMultiInserter("issuedNames", []string{"reversedName", "serial", "notBefore", "renewal"}, "")
+	multiInserter, err := db.NewMultiInserter("issuedNames", []string{"reversedName", "serial", "notBefore", "renewal"})
 	if err != nil {
 		return err
 	}
@@ -1008,8 +1008,7 @@ func addIssuedNames(ctx context.Context, queryer db.Queryer, cert *x509.Certific
 			return err
 		}
 	}
-	_, err = multiInserter.Insert(ctx, queryer)
+	return multiInserter.Insert(ctx, queryer)
-	return err
 }
 
 func addKeyHash(ctx context.Context, db db.Inserter, cert *x509.Certificate) error {

sa/sa.go

@@ -7,7 +7,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/go-jose/go-jose/v4"
@@ -21,7 +20,6 @@ import (
 	corepb "github.com/letsencrypt/boulder/core/proto"
 	"github.com/letsencrypt/boulder/db"
 	berrors "github.com/letsencrypt/boulder/errors"
-	"github.com/letsencrypt/boulder/features"
 	bgrpc "github.com/letsencrypt/boulder/grpc"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/revocation"
@@ -518,8 +516,7 @@ 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)
+		newAuthzIDs := make([]int64, 0, len(req.NewAuthzs))
-		if features.Get().InsertAuthzsIndividually {
 		for _, authz := range req.NewAuthzs {
 			am, err := newAuthzReqToModel(authz, req.NewOrder.CertificateProfileName)
 			if err != nil {
@@ -531,42 +528,6 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 			}
 			newAuthzIDs = append(newAuthzIDs, am.ID)
 		}
-		} 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, req.NewOrder.CertificateProfileName)
-					if err != nil {
-						return nil, err
-					}
-					err = inserter.Add([]interface{}{
-						am.ID,
-						am.IdentifierType,
-						am.IdentifierValue,
-						am.RegistrationID,
-						am.CertificateProfileName,
-						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
-				}
-			}
-		}
 
 		// Second, insert the new order.
 		created := ssa.clk.Now()
@@ -585,7 +546,7 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 		// Third, insert all of the orderToAuthz relations.
 		// Have to combine the already-associated and newly-created authzs.
 		allAuthzIds := append(req.NewOrder.V2Authorizations, newAuthzIDs...)
-		inserter, err := db.NewMultiInserter("orderToAuthz2", []string{"orderID", "authzID"}, "")
+		inserter, err := db.NewMultiInserter("orderToAuthz2", []string{"orderID", "authzID"})
 		if err != nil {
 			return nil, err
 		}
@@ -595,7 +556,7 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 				return nil, err
 			}
 		}
-		_, err = inserter.Insert(ctx, tx)
+		err = inserter.Insert(ctx, tx)
 		if err != nil {
 			return nil, err
 		}

sa/sa_test.go

@@ -10,7 +10,6 @@ import (
 	"crypto/x509"
 	"database/sql"
 	"encoding/base64"
-	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -722,15 +721,28 @@ func TestFQDNSetsExists(t *testing.T) {
 	test.Assert(t, exists.Exists, "FQDN set does exist")
 }
 
-type queryRecorder struct {
+type execRecorder struct {
+	valuesPerRow int
 	query        string
 	args         []interface{}
 }
 
-func (e *queryRecorder) QueryContext(ctx context.Context, query string, args ...interface{}) (*sql.Rows, error) {
+func (e *execRecorder) ExecContext(ctx context.Context, query string, args ...interface{}) (sql.Result, error) {
 	e.query = query
 	e.args = args
-	return nil, nil
+	return rowsResult{int64(len(args) / e.valuesPerRow)}, nil
+}
+
+type rowsResult struct {
+	rowsAffected int64
+}
+
+func (r rowsResult) LastInsertId() (int64, error) {
+	return r.rowsAffected, nil
+}
+
+func (r rowsResult) RowsAffected() (int64, error) {
+	return r.rowsAffected, nil
 }
 
 func TestAddIssuedNames(t *testing.T) {
@@ -813,7 +825,7 @@ func TestAddIssuedNames(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.Name, func(t *testing.T) {
-			var e queryRecorder
+			e := execRecorder{valuesPerRow: 4}
 			err := addIssuedNames(
 				ctx,
 				&e,
@@ -891,9 +903,6 @@ 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
@@ -948,9 +957,6 @@ 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)
@@ -972,9 +978,6 @@ 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,
@@ -993,9 +996,6 @@ 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"
@@ -1093,55 +1093,6 @@ func TestNewOrderAndAuthzs_Profile(t *testing.T) {
 	}
 }
 
-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()

test/config-next/sa.json

@@ -48,9 +48,7 @@
 			}
 		},
 		"healthCheckInterval": "4s",
-		"features": {
+		"features": {}
-			"InsertAuthzsIndividually": true
-		}
 	},
 	"syslog": {
 		"stdoutlevel": 6,

test/config/sa.json

@@ -49,7 +49,8 @@
 		},
 		"features": {
 			"UseKvLimitsForNewOrder": true,
-			"MultipleCertificateProfiles": true
+			"MultipleCertificateProfiles": true,
+			"InsertAuthzsIndividually": true
 		}
 	},
 	"syslog": {