From ea76eb2bebc6d49ea65415beeafb44618fb94ede Mon Sep 17 00:00:00 2001 From: "A.J. Beamon" Date: Fri, 29 Jul 2022 19:36:56 -0700 Subject: [PATCH] Transactions could change tenant ID part way through. The dummy transaction could set a new tenant ID but use the old tenant prefix. --- fdbclient/NativeAPI.actor.cpp | 37 +++++++++++++++++---------- fdbserver/CommitProxyServer.actor.cpp | 34 +++++++++++++++--------- 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/fdbclient/NativeAPI.actor.cpp b/fdbclient/NativeAPI.actor.cpp index dcf86db793..7ef869c4e9 100644 --- a/fdbclient/NativeAPI.actor.cpp +++ b/fdbclient/NativeAPI.actor.cpp @@ -2992,9 +2992,11 @@ Future getKeyLocation(Reference trState, isBackward, version); - if (trState->tenant().present() && useTenant) { + if (trState->tenant().present() && useTenant && trState->tenantId == TenantInfo::INVALID_TENANT) { return map(f, [trState](const KeyRangeLocationInfo& locationInfo) { - trState->tenantId = locationInfo.tenantEntry.id; + if (trState->tenantId == TenantInfo::INVALID_TENANT) { + trState->tenantId = locationInfo.tenantEntry.id; + } return locationInfo; }); } else { @@ -3132,10 +3134,12 @@ Future> getKeyRangeLocations(ReferenceuseProvisionalProxies, version); - if (trState->tenant().present() && useTenant) { + if (trState->tenant().present() && useTenant && trState->tenantId == TenantInfo::INVALID_TENANT) { return map(f, [trState](const std::vector& locationInfo) { ASSERT(!locationInfo.empty()); - trState->tenantId = locationInfo[0].tenantEntry.id; + if (trState->tenantId == TenantInfo::INVALID_TENANT) { + trState->tenantId = locationInfo[0].tenantEntry.id; + } return locationInfo; }); } else { @@ -5972,6 +5976,7 @@ ACTOR static Future commitDummyTransaction(Reference trS tr.trState->options = trState->options; tr.trState->taskID = trState->taskID; tr.trState->authToken = trState->authToken; + tr.trState->tenantId = trState->tenantId; if (!trState->hasTenant()) { tr.setOption(FDBTransactionOptions::RAW_ACCESS); } else { @@ -5985,6 +5990,10 @@ ACTOR static Future commitDummyTransaction(Reference trS wait(tr.commit()); return Void(); } catch (Error& e) { + // If the tenant is gone, then our original transaction won't be able to commit + if (e.code() == error_code_unknown_tenant) { + return Void(); + } TraceEvent("CommitDummyTransactionError") .errorUnsuppressed(e) .detail("Key", range.begin) @@ -6157,19 +6166,17 @@ ACTOR static Future tryCommit(Reference trState, } state Key tenantPrefix; - if (trState->tenant().present()) { + // skipApplyTenantPrefix is set only in the context of a commitDummyTransaction() + // (see member declaration) + if (trState->tenant().present() && !trState->skipApplyTenantPrefix) { KeyRangeLocationInfo locationInfo = wait(getKeyLocation(trState, ""_sr, &StorageServerInterface::getValue, Reverse::False, UseTenant::True, req.transaction.read_snapshot)); - // skipApplyTenantPrefix is set only in the context of a commitDummyTransaction() - // (see member declaration) - if (!trState->skipApplyTenantPrefix) { - applyTenantPrefix(req, locationInfo.tenantEntry.prefix); - tenantPrefixPrepended = TenantPrefixPrepended::True; - } + applyTenantPrefix(req, locationInfo.tenantEntry.prefix); + tenantPrefixPrepended = TenantPrefixPrepended::True; tenantPrefix = locationInfo.tenantEntry.prefix; } CODE_PROBE(trState->skipApplyTenantPrefix, "Tenant prefix prepend skipped for dummy transaction"); @@ -7621,10 +7628,14 @@ ACTOR Future blobGranuleGetTenantEntry(Transaction* self, Key ra self->trState->useProvisionalProxies, Reverse::False, latestVersion)); - self->trState->tenantId = l.tenantEntry.id; + if (self->trState->tenantId == TenantInfo::INVALID_TENANT) { + self->trState->tenantId = l.tenantEntry.id; + } return l.tenantEntry; } else { - self->trState->tenantId = cachedLocationInfo.get().tenantEntry.id; + if (self->trState->tenantId == TenantInfo::INVALID_TENANT) { + self->trState->tenantId = cachedLocationInfo.get().tenantEntry.id; + } return cachedLocationInfo.get().tenantEntry; } } diff --git a/fdbserver/CommitProxyServer.actor.cpp b/fdbserver/CommitProxyServer.actor.cpp index 7f0b698faa..9da19b6529 100644 --- a/fdbserver/CommitProxyServer.actor.cpp +++ b/fdbserver/CommitProxyServer.actor.cpp @@ -283,16 +283,20 @@ bool verifyTenantPrefix(ProxyCommitData* const commitData, const CommitTransacti if (!m.param1.startsWith(tenantPrefix)) { TraceEvent(SevWarnAlways, "TenantPrefixMismatch") .suppressFor(60) - .detail("Prefix", tenantPrefix.toHexString()) - .detail("Key", m.param1.toHexString()); + .detail("Tenant", req.tenantInfo.name) + .detail("TenantID", req.tenantInfo.tenantId) + .detail("Prefix", tenantPrefix) + .detail("Key", m.param1); return false; } if (m.type == MutationRef::ClearRange && !m.param2.startsWith(tenantPrefix)) { TraceEvent(SevWarnAlways, "TenantClearRangePrefixMismatch") .suppressFor(60) - .detail("Prefix", tenantPrefix.toHexString()) - .detail("Key", m.param2.toHexString()); + .detail("Tenant", req.tenantInfo.name) + .detail("TenantID", req.tenantInfo.tenantId) + .detail("Prefix", tenantPrefix) + .detail("Key", m.param2); return false; } else if (m.type == MutationRef::SetVersionstampedKey) { ASSERT(m.param1.size() >= 4); @@ -301,8 +305,10 @@ bool verifyTenantPrefix(ProxyCommitData* const commitData, const CommitTransacti if (*offset < tenantPrefix.size()) { TraceEvent(SevWarnAlways, "TenantVersionstampInvalidOffset") .suppressFor(60) - .detail("Prefix", tenantPrefix.toHexString()) - .detail("Key", m.param1.toHexString()) + .detail("Tenant", req.tenantInfo.name) + .detail("TenantID", req.tenantInfo.tenantId) + .detail("Prefix", tenantPrefix) + .detail("Key", m.param1) .detail("Offset", *offset); return false; } @@ -315,9 +321,11 @@ bool verifyTenantPrefix(ProxyCommitData* const commitData, const CommitTransacti (!rc.begin.startsWith(tenantPrefix) || !rc.end.startsWith(tenantPrefix))) { TraceEvent(SevWarnAlways, "TenantReadConflictPrefixMismatch") .suppressFor(60) - .detail("Prefix", tenantPrefix.toHexString()) - .detail("BeginKey", rc.begin.toHexString()) - .detail("EndKey", rc.end.toHexString()); + .detail("Tenant", req.tenantInfo.name) + .detail("TenantID", req.tenantInfo.tenantId) + .detail("Prefix", tenantPrefix) + .detail("BeginKey", rc.begin) + .detail("EndKey", rc.end); return false; } } @@ -327,9 +335,11 @@ bool verifyTenantPrefix(ProxyCommitData* const commitData, const CommitTransacti (!wc.begin.startsWith(tenantPrefix) || !wc.end.startsWith(tenantPrefix))) { TraceEvent(SevWarnAlways, "TenantWriteConflictPrefixMismatch") .suppressFor(60) - .detail("Prefix", tenantPrefix.toHexString()) - .detail("BeginKey", wc.begin.toHexString()) - .detail("EndKey", wc.end.toHexString()); + .detail("Tenant", req.tenantInfo.name) + .detail("TenantID", req.tenantInfo.tenantId) + .detail("Prefix", tenantPrefix) + .detail("BeginKey", wc.begin) + .detail("EndKey", wc.end); return false; } }