From 2131065b2d0393fdbd21a5060c021873f2cb5121 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 9 Jul 2019 14:33:28 -0700 Subject: [PATCH] Revert "SA: improve performance of GetOrderForNames. (#4326)" (#4328) This reverts commit 9fa360769ec0d24aad78d77ae774b3a7996a4947. This commit can cause "gorp: multiple rows returned for: ..." under certain situations. See #4329 for details of followup. --- features/featureflag_string.go | 5 ++-- features/features.go | 3 -- sa/sa.go | 53 ++++++++-------------------------- test/config-next/sa.json | 1 - 4 files changed, 14 insertions(+), 48 deletions(-) diff --git a/features/featureflag_string.go b/features/featureflag_string.go index 7d873b5e4..54ea7110e 100644 --- a/features/featureflag_string.go +++ b/features/featureflag_string.go @@ -29,12 +29,11 @@ func _() { _ = x[RemoveWFE2AccountID-18] _ = x[CheckRenewalFirst-19] _ = x[MandatoryPOSTAsGET-20] - _ = x[FasterGetOrderForNames-21] } -const _FeatureFlag_name = "unusedPerformValidationRPCACME13KeyRolloverSimplifiedVAHTTPTLSSNIRevalidationAllowRenewalFirstRLSetIssuedNamesRenewalBitFasterRateLimitProbeCTLogsRevokeAtRACAAValidationMethodsCAAAccountURIHeadNonceStatusOKNewAuthorizationSchemaDisableAuthz2OrdersEarlyOrderRateLimitEnforceMultiVAMultiVAFullResultsRemoveWFE2AccountIDCheckRenewalFirstMandatoryPOSTAsGETFasterGetOrderForNames" +const _FeatureFlag_name = "unusedPerformValidationRPCACME13KeyRolloverSimplifiedVAHTTPTLSSNIRevalidationAllowRenewalFirstRLSetIssuedNamesRenewalBitFasterRateLimitProbeCTLogsRevokeAtRACAAValidationMethodsCAAAccountURIHeadNonceStatusOKNewAuthorizationSchemaDisableAuthz2OrdersEarlyOrderRateLimitEnforceMultiVAMultiVAFullResultsRemoveWFE2AccountIDCheckRenewalFirstMandatoryPOSTAsGET" -var _FeatureFlag_index = [...]uint16{0, 6, 26, 43, 59, 77, 96, 120, 135, 146, 156, 176, 189, 206, 228, 247, 266, 280, 298, 317, 334, 352, 374} +var _FeatureFlag_index = [...]uint16{0, 6, 26, 43, 59, 77, 96, 120, 135, 146, 156, 176, 189, 206, 228, 247, 266, 280, 298, 317, 334, 352} func (i FeatureFlag) String() string { if i < 0 || i >= FeatureFlag(len(_FeatureFlag_index)-1) { diff --git a/features/features.go b/features/features.go index cad796554..eaf679077 100644 --- a/features/features.go +++ b/features/features.go @@ -57,8 +57,6 @@ const ( // MandatoryPOSTAsGET forbids legacy unauthenticated GET requests for ACME // resources. MandatoryPOSTAsGET - // Use an optimized query for GetOrderForNames. - FasterGetOrderForNames ) // List of features and their default value, protected by fMu @@ -84,7 +82,6 @@ var features = map[FeatureFlag]bool{ CheckRenewalFirst: false, MandatoryPOSTAsGET: false, DisableAuthz2Orders: false, - FasterGetOrderForNames: false, } var fMu = new(sync.RWMutex) diff --git a/sa/sa.go b/sa/sa.go index 2310aa285..f054bb538 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -1812,58 +1812,29 @@ func (ssa *SQLStorageAuthority) GetOrderForNames( // Hash the names requested for lookup in the orderFqdnSets table fqdnHash := hashNames(req.Names) - // Find a possibly-suitable order. We don't include the account ID or order - // status in this query because there's no index that includes those, so - // including them could require the DB to scan extra rows. - // Instead, we select one unexpired order that matches the fqdnSet. If - // that order doesn't match the account ID or status we need, just return - // nothing. We use `ORDER BY expires ASC` because the index on - // (setHash, expires) is in ASC order. DESC would be slightly nicer from a - // user experience perspective but would be slow when there are many entries - // to sort. - // This approach works fine because in most cases there's only one account - // issuing for a given name. If there are other accounts issuing for the same - // name, it just means order reuse happens less often. - var result struct { - OrderID int64 - RegistrationID int64 - } - var err error - if features.Enabled(features.FasterGetOrderForNames) { - err = ssa.dbMap.WithContext(ctx).SelectOne(&result, ` - SELECT orderID, registrationID - FROM orderFqdnSets - WHERE setHash = ? - AND expires > ? - ORDER BY expires DESC - LIMIT 1`, - fqdnHash, ssa.clk.Now()) - } else { - err = ssa.dbMap.WithContext(ctx).SelectOne(&result, ` - SELECT orderID, registrationID - FROM orderFqdnSets - WHERE setHash = ? - AND registrationID = ? - AND expires > ?`, - fqdnHash, *req.AcctID, ssa.clk.Now()) - } + var orderID int64 + err := ssa.dbMap.WithContext(ctx).SelectOne(&orderID, ` + SELECT orderID + FROM orderFqdnSets + WHERE setHash = ? + AND registrationID = ? + AND expires > ?`, + fqdnHash, *req.AcctID, ssa.clk.Now()) + // There isn't an unexpired order for the provided AcctID that has the + // fqdnHash requested. if err == sql.ErrNoRows { return nil, berrors.NotFoundError("no order matching request found") } else if err != nil { + // An unexpected error occurred return nil, err } - if result.RegistrationID != *req.AcctID { - return nil, berrors.NotFoundError("no order matching request found") - } - // Get the order - order, err := ssa.GetOrder(ctx, &sapb.OrderRequest{Id: &result.OrderID, UseV2Authorizations: req.UseV2Authorizations}) + order, err := ssa.GetOrder(ctx, &sapb.OrderRequest{Id: &orderID, UseV2Authorizations: req.UseV2Authorizations}) if err != nil { return nil, err } - // Only return a pending or ready order if *order.Status != string(core.StatusPending) && *order.Status != string(core.StatusReady) { diff --git a/test/config-next/sa.json b/test/config-next/sa.json index c0cfe755e..0b7414c3b 100644 --- a/test/config-next/sa.json +++ b/test/config-next/sa.json @@ -24,7 +24,6 @@ ] }, "features": { - "FasterGetOrderForNames": true } },