From 62b5e8b326b76155dcccb4324164645fa9abb846 Mon Sep 17 00:00:00 2001 From: Alex Sarkesian Date: Tue, 9 Aug 2022 15:52:07 -0400 Subject: [PATCH 1/2] kvserver: track replicate queue metrics by allocator action While previously we had metrics within the replicate queue which tracked the number of particular actions processed by the queue based on a unique set of categories, this change adds new metrics for tracking the successes/errors of a replica being processed by the replicate queue, using the allocator action as a method of categorizing these actions. With this categorization, we are able to track success and error counts during rebalancing, upreplicating when we have a dead node, or decommissioning. The categorization makes no distinction between actions relatinv to voter replicas vs non-voter replicas, so they are aggregated across these two types. Release note (ops change): added new metrics: ``` queue.replicate.addreplica.(success|error) queue.replicate.removereplica.(success|error) queue.replicate.replacedeadreplica.(success|error) queue.replicate.removedeadreplica.(success|error) queue.replicate.replacedecommissioningreplica.(success|error) queue.replicate.removedecommissioningreplica.(success|error) ``` --- pkg/kv/kvserver/replicate_queue.go | 182 ++++++++++++++++++++++-- pkg/kv/kvserver/replicate_queue_test.go | 48 +++++++ pkg/ts/catalog/chart_catalog.go | 22 +++ 3 files changed, 244 insertions(+), 8 deletions(-) diff --git a/pkg/kv/kvserver/replicate_queue.go b/pkg/kv/kvserver/replicate_queue.go index f4178dedbe81..0d8123d9a15b 100644 --- a/pkg/kv/kvserver/replicate_queue.go +++ b/pkg/kv/kvserver/replicate_queue.go @@ -186,6 +186,78 @@ var ( Measurement: "Demotions of Voters to Non Voters", Unit: metric.Unit_COUNT, } + metaReplicateQueueAddReplicaSuccessCount = metric.Metadata{ + Name: "queue.replicate.addreplica.success", + Help: "Number of successful replica additions processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueAddReplicaErrorCount = metric.Metadata{ + Name: "queue.replicate.addreplica.error", + Help: "Number of failed replica additions processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueRemoveReplicaSuccessCount = metric.Metadata{ + Name: "queue.replicate.removereplica.success", + Help: "Number of successful replica removals processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueRemoveReplicaErrorCount = metric.Metadata{ + Name: "queue.replicate.removereplica.error", + Help: "Number of failed replica removals processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueReplaceDeadReplicaSuccessCount = metric.Metadata{ + Name: "queue.replicate.replacedeadreplica.success", + Help: "Number of successful dead replica replica replacements processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueReplaceDeadReplicaErrorCount = metric.Metadata{ + Name: "queue.replicate.replacedeadreplica.error", + Help: "Number of failed dead replica replica replacements processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueReplaceDecommissioningReplicaSuccessCount = metric.Metadata{ + Name: "queue.replicate.replacedecommissioningreplica.success", + Help: "Number of successful decommissioning replica replica replacements processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueReplaceDecommissioningReplicaErrorCount = metric.Metadata{ + Name: "queue.replicate.replacedecommissioningreplica.error", + Help: "Number of failed decommissioning replica replica replacements processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueRemoveDecommissioningReplicaSuccessCount = metric.Metadata{ + Name: "queue.replicate.removedecommissioningreplica.success", + Help: "Number of successful decommissioning replica replica removals processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueRemoveDecommissioningReplicaErrorCount = metric.Metadata{ + Name: "queue.replicate.removedecommissioningreplica.error", + Help: "Number of failed decommissioning replica replica removals processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueRemoveDeadReplicaSuccessCount = metric.Metadata{ + Name: "queue.replicate.removedeadreplica.success", + Help: "Number of successful dead replica replica removals processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } + metaReplicateQueueRemoveDeadReplicaErrorCount = metric.Metadata{ + Name: "queue.replicate.removedeadreplica.error", + Help: "Number of failed dead replica replica removals processed by the replicate queue", + Measurement: "Replicas", + Unit: metric.Unit_COUNT, + } ) // quorumError indicates a retryable error condition which sends replicas being @@ -228,6 +300,23 @@ type ReplicateQueueMetrics struct { TransferLeaseCount *metric.Counter NonVoterPromotionsCount *metric.Counter VoterDemotionsCount *metric.Counter + + // Success/error counts by allocator action. + RemoveReplicaSuccessCount *metric.Counter + RemoveReplicaErrorCount *metric.Counter + AddReplicaSuccessCount *metric.Counter + AddReplicaErrorCount *metric.Counter + ReplaceDeadReplicaSuccessCount *metric.Counter + ReplaceDeadReplicaErrorCount *metric.Counter + RemoveDeadReplicaSuccessCount *metric.Counter + RemoveDeadReplicaErrorCount *metric.Counter + ReplaceDecommissioningReplicaSuccessCount *metric.Counter + ReplaceDecommissioningReplicaErrorCount *metric.Counter + RemoveDecommissioningReplicaSuccessCount *metric.Counter + RemoveDecommissioningReplicaErrorCount *metric.Counter + // TODO(sarkesian): Consider adding metrics for AllocatorRemoveLearner, + // AllocatorConsiderRebalance, and AllocatorFinalizeAtomicReplicationChange + // allocator actions. } func makeReplicateQueueMetrics() ReplicateQueueMetrics { @@ -251,6 +340,19 @@ func makeReplicateQueueMetrics() ReplicateQueueMetrics { TransferLeaseCount: metric.NewCounter(metaReplicateQueueTransferLeaseCount), NonVoterPromotionsCount: metric.NewCounter(metaReplicateQueueNonVoterPromotionsCount), VoterDemotionsCount: metric.NewCounter(metaReplicateQueueVoterDemotionsCount), + + RemoveReplicaSuccessCount: metric.NewCounter(metaReplicateQueueRemoveReplicaSuccessCount), + RemoveReplicaErrorCount: metric.NewCounter(metaReplicateQueueRemoveReplicaErrorCount), + AddReplicaSuccessCount: metric.NewCounter(metaReplicateQueueAddReplicaSuccessCount), + AddReplicaErrorCount: metric.NewCounter(metaReplicateQueueAddReplicaErrorCount), + ReplaceDeadReplicaSuccessCount: metric.NewCounter(metaReplicateQueueReplaceDeadReplicaSuccessCount), + ReplaceDeadReplicaErrorCount: metric.NewCounter(metaReplicateQueueReplaceDeadReplicaErrorCount), + RemoveDeadReplicaSuccessCount: metric.NewCounter(metaReplicateQueueRemoveDeadReplicaSuccessCount), + RemoveDeadReplicaErrorCount: metric.NewCounter(metaReplicateQueueRemoveDeadReplicaErrorCount), + ReplaceDecommissioningReplicaSuccessCount: metric.NewCounter(metaReplicateQueueReplaceDecommissioningReplicaSuccessCount), + ReplaceDecommissioningReplicaErrorCount: metric.NewCounter(metaReplicateQueueReplaceDecommissioningReplicaErrorCount), + RemoveDecommissioningReplicaSuccessCount: metric.NewCounter(metaReplicateQueueRemoveDecommissioningReplicaSuccessCount), + RemoveDecommissioningReplicaErrorCount: metric.NewCounter(metaReplicateQueueRemoveDecommissioningReplicaErrorCount), } } @@ -353,6 +455,50 @@ func (metrics *ReplicateQueueMetrics) trackRebalanceReplicaCount( } } +// trackProcessResult increases the corresponding success/error count metric for +// processing a particular allocator action through the replicate queue. +func (metrics *ReplicateQueueMetrics) trackResultByAllocatorAction( + action allocatorimpl.AllocatorAction, err error, dryRun bool, +) { + if dryRun { + return + } + switch action { + case allocatorimpl.AllocatorRemoveVoter, allocatorimpl.AllocatorRemoveNonVoter: + if err == nil { + metrics.RemoveReplicaSuccessCount.Inc(1) + } else { + metrics.RemoveReplicaErrorCount.Inc(1) + } + case allocatorimpl.AllocatorAddVoter, allocatorimpl.AllocatorAddNonVoter: + if err == nil { + metrics.AddReplicaSuccessCount.Inc(1) + } else { + metrics.AddReplicaErrorCount.Inc(1) + } + case allocatorimpl.AllocatorReplaceDeadVoter, allocatorimpl.AllocatorReplaceDeadNonVoter: + if err == nil { + metrics.ReplaceDeadReplicaSuccessCount.Inc(1) + } else { + metrics.ReplaceDeadReplicaErrorCount.Inc(1) + } + case allocatorimpl.AllocatorReplaceDecommissioningVoter, allocatorimpl.AllocatorReplaceDecommissioningNonVoter: + if err == nil { + metrics.ReplaceDecommissioningReplicaSuccessCount.Inc(1) + } else { + metrics.ReplaceDecommissioningReplicaErrorCount.Inc(1) + } + case allocatorimpl.AllocatorRemoveDecommissioningVoter, allocatorimpl.AllocatorRemoveDecommissioningNonVoter: + if err == nil { + metrics.RemoveDecommissioningReplicaSuccessCount.Inc(1) + } else { + metrics.RemoveDecommissioningReplicaErrorCount.Inc(1) + } + default: + panic(fmt.Sprintf("unsupported AllocatorAction: %v", action)) + } +} + // replicateQueue manages a queue of replicas which may need to add an // additional replica to their range. type replicateQueue struct { @@ -641,19 +787,27 @@ func (rq *replicateQueue) processOneChange( // Add replicas. case allocatorimpl.AllocatorAddVoter: - return rq.addOrReplaceVoters( + requeue, err := rq.addOrReplaceVoters( ctx, repl, liveVoterReplicas, liveNonVoterReplicas, -1 /* removeIdx */, allocatorimpl.Alive, dryRun, ) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) + return requeue, err case allocatorimpl.AllocatorAddNonVoter: - return rq.addOrReplaceNonVoters( + requeue, err := rq.addOrReplaceNonVoters( ctx, repl, liveVoterReplicas, liveNonVoterReplicas, -1 /* removeIdx */, allocatorimpl.Alive, dryRun, ) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) + return requeue, err // Remove replicas. case allocatorimpl.AllocatorRemoveVoter: - return rq.removeVoter(ctx, repl, voterReplicas, nonVoterReplicas, dryRun) + requeue, err := rq.removeVoter(ctx, repl, voterReplicas, nonVoterReplicas, dryRun) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) + return requeue, err case allocatorimpl.AllocatorRemoveNonVoter: - return rq.removeNonVoter(ctx, repl, voterReplicas, nonVoterReplicas, dryRun) + requeue, err := rq.removeNonVoter(ctx, repl, voterReplicas, nonVoterReplicas, dryRun) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) + return requeue, err // Replace dead replicas. case allocatorimpl.AllocatorReplaceDeadVoter: @@ -667,8 +821,10 @@ func (rq *replicateQueue) processOneChange( "dead voter %v unexpectedly not found in %v", deadVoterReplicas[0], voterReplicas) } - return rq.addOrReplaceVoters( + requeue, err := rq.addOrReplaceVoters( ctx, repl, liveVoterReplicas, liveNonVoterReplicas, removeIdx, allocatorimpl.Dead, dryRun) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) + return requeue, err case allocatorimpl.AllocatorReplaceDeadNonVoter: if len(deadNonVoterReplicas) == 0 { // Nothing to do. @@ -680,8 +836,10 @@ func (rq *replicateQueue) processOneChange( "dead non-voter %v unexpectedly not found in %v", deadNonVoterReplicas[0], nonVoterReplicas) } - return rq.addOrReplaceNonVoters( + requeue, err := rq.addOrReplaceNonVoters( ctx, repl, liveVoterReplicas, liveNonVoterReplicas, removeIdx, allocatorimpl.Dead, dryRun) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) + return requeue, err // Replace decommissioning replicas. case allocatorimpl.AllocatorReplaceDecommissioningVoter: @@ -698,6 +856,7 @@ func (rq *replicateQueue) processOneChange( } requeue, err := rq.addOrReplaceVoters( ctx, repl, liveVoterReplicas, liveNonVoterReplicas, removeIdx, allocatorimpl.Decommissioning, dryRun) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) if err != nil { return requeue, decommissionPurgatoryError{err} } @@ -715,6 +874,7 @@ func (rq *replicateQueue) processOneChange( } requeue, err := rq.addOrReplaceNonVoters( ctx, repl, liveVoterReplicas, liveNonVoterReplicas, removeIdx, allocatorimpl.Decommissioning, dryRun) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) if err != nil { return requeue, decommissionPurgatoryError{err} } @@ -727,12 +887,14 @@ func (rq *replicateQueue) processOneChange( // AllocatorReplaceDecommissioning{Non}Voter above. case allocatorimpl.AllocatorRemoveDecommissioningVoter: requeue, err := rq.removeDecommissioning(ctx, repl, allocatorimpl.VoterTarget, dryRun) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) if err != nil { return requeue, decommissionPurgatoryError{err} } return requeue, nil case allocatorimpl.AllocatorRemoveDecommissioningNonVoter: requeue, err := rq.removeDecommissioning(ctx, repl, allocatorimpl.NonVoterTarget, dryRun) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) if err != nil { return requeue, decommissionPurgatoryError{err} } @@ -744,9 +906,13 @@ func (rq *replicateQueue) processOneChange( // over-replicated and has dead replicas; in the common case we'll hit // AllocatorReplaceDead{Non}Voter above. case allocatorimpl.AllocatorRemoveDeadVoter: - return rq.removeDead(ctx, repl, deadVoterReplicas, allocatorimpl.VoterTarget, dryRun) + requeue, err := rq.removeDead(ctx, repl, deadVoterReplicas, allocatorimpl.VoterTarget, dryRun) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) + return requeue, err case allocatorimpl.AllocatorRemoveDeadNonVoter: - return rq.removeDead(ctx, repl, deadNonVoterReplicas, allocatorimpl.NonVoterTarget, dryRun) + requeue, err := rq.removeDead(ctx, repl, deadNonVoterReplicas, allocatorimpl.NonVoterTarget, dryRun) + rq.metrics.trackResultByAllocatorAction(action, err, dryRun) + return requeue, err case allocatorimpl.AllocatorConsiderRebalance: return rq.considerRebalance( diff --git a/pkg/kv/kvserver/replicate_queue_test.go b/pkg/kv/kvserver/replicate_queue_test.go index b2fd78760175..6c5827963b0b 100644 --- a/pkg/kv/kvserver/replicate_queue_test.go +++ b/pkg/kv/kvserver/replicate_queue_test.go @@ -480,6 +480,8 @@ func TestReplicateQueueDecommissioningNonVoters(t *testing.T) { previousRemovalCount := store.ReplicateQueueMetrics().RemoveNonVoterReplicaCount.Count() previousDecommRemovals := store.ReplicateQueueMetrics().RemoveDecommissioningNonVoterReplicaCount.Count() + previousDecommReplacementSuccesses := + store.ReplicateQueueMetrics().ReplaceDecommissioningReplicaSuccessCount.Count() // Decommission each of the two nodes that have the non-voters and make sure // that those non-voters are upreplicated elsewhere. @@ -515,6 +517,8 @@ func TestReplicateQueueDecommissioningNonVoters(t *testing.T) { currentRemoveCount := store.ReplicateQueueMetrics().RemoveNonVoterReplicaCount.Count() currentDecommRemovals := store.ReplicateQueueMetrics().RemoveDecommissioningNonVoterReplicaCount.Count() + currentDecommReplacementSuccesses := + store.ReplicateQueueMetrics().ReplaceDecommissioningReplicaSuccessCount.Count() require.GreaterOrEqualf( t, currentAddCount, previousAddCount+2, @@ -528,6 +532,9 @@ func TestReplicateQueueDecommissioningNonVoters(t *testing.T) { t, currentDecommRemovals, previousDecommRemovals+2, "expected decommissioning replica removals to increase by at least 2", ) + require.GreaterOrEqualf(t, currentDecommReplacementSuccesses, previousDecommReplacementSuccesses+2, + "expected decommissioning replica replacement successes to increase by at least 2", + ) }) // Check that when we have more non-voters than needed and some of those @@ -558,9 +565,22 @@ func TestReplicateQueueDecommissioningNonVoters(t *testing.T) { if err != nil { t.Fatal(err) } + + // Ensure leaseholder has updated span config with 0 non-voters. + require.Eventually(t, func() bool { + repl, err := store.GetReplica(scratchRange.RangeID) + if err != nil { + t.Fatal(err) + } + _, conf := repl.DescAndSpanConfig() + return conf.GetNumNonVoters() == 0 + }, testutils.DefaultSucceedsSoonDuration, 100*time.Millisecond) + previousRemovalCount := store.ReplicateQueueMetrics().RemoveNonVoterReplicaCount.Count() previousDecommRemovals := store.ReplicateQueueMetrics().RemoveDecommissioningNonVoterReplicaCount.Count() + previousDecommRemovalSuccesses := + store.ReplicateQueueMetrics().RemoveDecommissioningReplicaSuccessCount.Count() require.NoError(t, tc.Server(0).Decommission(ctx, livenesspb.MembershipStatus_DECOMMISSIONING, nonVoterNodeIDs)) @@ -581,6 +601,8 @@ func TestReplicateQueueDecommissioningNonVoters(t *testing.T) { currentRemoveCount := store.ReplicateQueueMetrics().RemoveNonVoterReplicaCount.Count() currentDecommRemovals := store.ReplicateQueueMetrics().RemoveDecommissioningNonVoterReplicaCount.Count() + currentDecommRemovalSuccesses := + store.ReplicateQueueMetrics().RemoveDecommissioningReplicaSuccessCount.Count() require.GreaterOrEqualf( t, currentRemoveCount, previousRemovalCount+2, "expected replica removals to increase by at least 2", @@ -589,6 +611,9 @@ func TestReplicateQueueDecommissioningNonVoters(t *testing.T) { t, currentDecommRemovals, previousDecommRemovals+2, "expected replica removals to increase by at least 2", ) + require.GreaterOrEqualf(t, currentDecommRemovalSuccesses, previousDecommRemovalSuccesses+2, + "expected decommissioning replica removal successes to increase by at least 2", + ) }) } @@ -765,6 +790,7 @@ func TestReplicateQueueDeadNonVoters(t *testing.T) { prevAdditions := store.ReplicateQueueMetrics().AddNonVoterReplicaCount.Count() prevRemovals := store.ReplicateQueueMetrics().RemoveNonVoterReplicaCount.Count() prevDeadRemovals := store.ReplicateQueueMetrics().RemoveDeadNonVoterReplicaCount.Count() + prevDeadReplacementSuccesses := store.ReplicateQueueMetrics().ReplaceDeadReplicaSuccessCount.Count() beforeNodeIDs := getNonVoterNodeIDs(scratchRange) markDead(beforeNodeIDs) @@ -794,6 +820,7 @@ func TestReplicateQueueDeadNonVoters(t *testing.T) { addCount := store.ReplicateQueueMetrics().AddNonVoterReplicaCount.Count() removeNonVoterCount := store.ReplicateQueueMetrics().RemoveNonVoterReplicaCount.Count() removeDeadNonVoterCount := store.ReplicateQueueMetrics().RemoveDeadNonVoterReplicaCount.Count() + replaceDeadSuccesses := store.ReplicateQueueMetrics().ReplaceDeadReplicaSuccessCount.Count() require.GreaterOrEqualf( t, addCount, prevAdditions+2, @@ -807,6 +834,10 @@ func TestReplicateQueueDeadNonVoters(t *testing.T) { t, removeDeadNonVoterCount, prevDeadRemovals+2, "expected replica removals to increase by at least 2", ) + require.GreaterOrEqualf( + t, replaceDeadSuccesses, prevDeadReplacementSuccesses+2, + "expected dead replica replacement successes to increase by at least 2", + ) }) // This subtest checks that when we have more non-voters than needed and some @@ -838,9 +869,21 @@ func TestReplicateQueueDeadNonVoters(t *testing.T) { if err != nil { t.Fatal(err) } + + // Ensure leaseholder has updated span config with 0 non-voters. + require.Eventually(t, func() bool { + repl, err := store.GetReplica(scratchRange.RangeID) + if err != nil { + t.Fatal(err) + } + _, conf := repl.DescAndSpanConfig() + return conf.GetNumNonVoters() == 0 + }, testutils.DefaultSucceedsSoonDuration, 100*time.Millisecond) + prevRemovals := store.ReplicateQueueMetrics().RemoveReplicaCount.Count() prevNonVoterRemovals := store.ReplicateQueueMetrics().RemoveNonVoterReplicaCount.Count() prevDeadRemovals := store.ReplicateQueueMetrics().RemoveDeadNonVoterReplicaCount.Count() + prevDeadRemovalSuccesses := store.ReplicateQueueMetrics().RemoveDeadReplicaSuccessCount.Count() beforeNodeIDs := getNonVoterNodeIDs(scratchRange) markDead(beforeNodeIDs) @@ -858,6 +901,7 @@ func TestReplicateQueueDeadNonVoters(t *testing.T) { removeCount := store.ReplicateQueueMetrics().RemoveReplicaCount.Count() removeNonVoterCount := store.ReplicateQueueMetrics().RemoveNonVoterReplicaCount.Count() removeDeadNonVoterCount := store.ReplicateQueueMetrics().RemoveDeadNonVoterReplicaCount.Count() + removeDeadSuccesses := store.ReplicateQueueMetrics().RemoveDeadReplicaSuccessCount.Count() require.GreaterOrEqualf( t, removeCount, prevRemovals+2, "expected replica removals to increase by at least 2", @@ -870,6 +914,10 @@ func TestReplicateQueueDeadNonVoters(t *testing.T) { t, removeDeadNonVoterCount, prevDeadRemovals+2, "expected replica removals to increase by at least 2", ) + require.GreaterOrEqualf( + t, removeDeadSuccesses, prevDeadRemovalSuccesses+2, + "expected dead replica removal successes to increase by at least 2", + ) }) } diff --git a/pkg/ts/catalog/chart_catalog.go b/pkg/ts/catalog/chart_catalog.go index 3be998121eaf..f37b4e6d758b 100644 --- a/pkg/ts/catalog/chart_catalog.go +++ b/pkg/ts/catalog/chart_catalog.go @@ -2020,6 +2020,28 @@ var charts = []sectionDescription{ "queue.replicate.removedecommissioningnonvoterreplica", }, }, + { + Title: "Successes by Action", + Metrics: []string{ + "queue.replicate.addreplica.success", + "queue.replicate.removereplica.success", + "queue.replicate.replacedeadreplica.success", + "queue.replicate.removedeadreplica.success", + "queue.replicate.replacedecommissioningreplica.success", + "queue.replicate.removedecommissioningreplica.success", + }, + }, + { + Title: "Errors by Action", + Metrics: []string{ + "queue.replicate.addreplica.error", + "queue.replicate.removereplica.error", + "queue.replicate.replacedeadreplica.error", + "queue.replicate.removedeadreplica.error", + "queue.replicate.replacedecommissioningreplica.error", + "queue.replicate.removedecommissioningreplica.error", + }, + }, { Title: "Successes", Metrics: []string{ From 0268864f3d7548804864c37564533ccfe1489c1c Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Wed, 10 Aug 2022 10:17:37 -0400 Subject: [PATCH 2/2] builtins: add trunc(decimal, int) builtin Release note (sql change): Added the trunc(decimal, int) builtin function, which truncates the given decimal value to the specified number of decimal places. A negative value can be used for the scale parameter, which will truncate to the left of the decimal point. Example: ``` > select trunc(541.234, 2), trunc(541.234, 0), trunc(541.234, -1); trunc | trunc | trunc ---------+-------+--------- 541.23 | 541 | 5.4E+2 ``` --- docs/generated/sql/functions.md | 2 + pkg/sql/BUILD.bazel | 1 + pkg/sql/function_resolver_test.go | 3 ++ .../testdata/logic_test/builtin_function | 17 +++++++++ .../logictest/testdata/logic_test/pg_catalog | 4 +- .../logictest/testdata/logic_test/pgoidtype | 4 +- pkg/sql/sem/builtins/math_builtins.go | 38 +++++++++++++++++++ 7 files changed, 65 insertions(+), 4 deletions(-) diff --git a/docs/generated/sql/functions.md b/docs/generated/sql/functions.md index 8636272ce93b..1da8d2b52fe9 100644 --- a/docs/generated/sql/functions.md +++ b/docs/generated/sql/functions.md @@ -871,6 +871,8 @@ available replica will error.

Immutable trunc(val: decimal) → decimal

Truncates the decimal values of val.

Immutable +trunc(val: decimal, scale: int) → decimal

Truncate val to scale decimal places

+Immutable trunc(val: float) → float

Truncates the decimal values of val.

Immutable diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel index 3112e96c38a3..89e1f94ccb4a 100644 --- a/pkg/sql/BUILD.bazel +++ b/pkg/sql/BUILD.bazel @@ -671,6 +671,7 @@ go_test( "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", "//pkg/sql/catalog/desctestutils", + "//pkg/sql/catalog/funcdesc", "//pkg/sql/catalog/lease", "//pkg/sql/catalog/multiregion", "//pkg/sql/catalog/schemadesc", diff --git a/pkg/sql/function_resolver_test.go b/pkg/sql/function_resolver_test.go index ab24df66489e..cf3543c2c841 100644 --- a/pkg/sql/function_resolver_test.go +++ b/pkg/sql/function_resolver_test.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc" "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -405,6 +406,8 @@ CREATE FUNCTION sc1.lower(a STRING) RETURNS STRING IMMUTABLE LANGUAGE SQL AS $$ require.False(t, funcExpr.ResolvedOverload().UDFContainsOnlySignature) if tc.expectedFuncOID > 0 { require.Equal(t, tc.expectedFuncOID, int(funcExpr.ResolvedOverload().Oid)) + } else { + require.False(t, funcdesc.IsOIDUserDefinedFunc(funcExpr.ResolvedOverload().Oid)) } require.Equal(t, tc.expectedFuncBody, funcExpr.ResolvedOverload().Body) }) diff --git a/pkg/sql/logictest/testdata/logic_test/builtin_function b/pkg/sql/logictest/testdata/logic_test/builtin_function index b759f6114076..43af1a6fa944 100644 --- a/pkg/sql/logictest/testdata/logic_test/builtin_function +++ b/pkg/sql/logictest/testdata/logic_test/builtin_function @@ -1249,6 +1249,23 @@ SELECT trunc(-0.0), trunc(0.0), trunc(1.9), trunc(19.5678::decimal) ---- 0 0 1 19 +query RRRRRRR +WITH v(x) AS + (VALUES('0'::numeric),('1'::numeric),('-1'::numeric),('4.2'::numeric), + ('-7.777'::numeric),('9127.777'::numeric),('inf'::numeric),('-inf'::numeric),('nan'::numeric)) +SELECT x, trunc(x), trunc(x,1), trunc(x,2), trunc(x,0), trunc(x,-1), trunc(x,-2) +FROM v +---- +0 0 0.0 0.00 0 0 0 +1 1 1.0 1.00 1 0 0 +-1 -1 -1.0 -1.00 -1 0 0 +4.2 4 4.2 4.20 4 0 0 +-7.777 -7 -7.7 -7.77 -7 0 0 +9127.777 9127 9127.7 9127.77 9127 9.12E+3 9.1E+3 +Infinity Infinity Infinity Infinity Infinity Infinity Infinity +-Infinity -Infinity -Infinity -Infinity -Infinity -Infinity -Infinity +NaN NaN NaN NaN NaN NaN NaN + query T SELECT translate('Techonthenet.com', 'e.to', '456') ---- diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index 45ac45e5c613..bbc0875b37a4 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -4645,7 +4645,7 @@ FROM pg_proc p JOIN pg_type t ON t.typinput = p.oid WHERE t.typname = '_int4' ---- -2011 array_in array_in +2012 array_in array_in ## #16285 ## int2vectors should be 0-indexed @@ -4683,7 +4683,7 @@ SELECT cur_max_builtin_oid FROM [SELECT max(oid) as cur_max_builtin_oid FROM pg_ query TT SELECT proname, oid FROM pg_catalog.pg_proc WHERE oid = $cur_max_builtin_oid ---- -to_regtype 2031 +to_regtype 2032 ## Ensure that unnest works with oid wrapper arrays diff --git a/pkg/sql/logictest/testdata/logic_test/pgoidtype b/pkg/sql/logictest/testdata/logic_test/pgoidtype index bc1ce3e7c70e..5f363e125bbc 100644 --- a/pkg/sql/logictest/testdata/logic_test/pgoidtype +++ b/pkg/sql/logictest/testdata/logic_test/pgoidtype @@ -83,7 +83,7 @@ WHERE relname = 'pg_constraint' query OOOO SELECT 'upper'::REGPROC, 'upper'::REGPROCEDURE, 'pg_catalog.upper'::REGPROCEDURE, 'upper'::REGPROC::OID ---- -upper upper upper 828 +upper upper upper 829 query error invalid function name SELECT 'invalid.more.pg_catalog.upper'::REGPROCEDURE @@ -91,7 +91,7 @@ SELECT 'invalid.more.pg_catalog.upper'::REGPROCEDURE query OOO SELECT 'upper(int)'::REGPROC, 'upper(int)'::REGPROCEDURE, 'upper(int)'::REGPROC::OID ---- -upper upper 828 +upper upper 829 query error unknown function: blah\(\) SELECT 'blah(ignored, ignored)'::REGPROC, 'blah(ignored, ignored)'::REGPROCEDURE diff --git a/pkg/sql/sem/builtins/math_builtins.go b/pkg/sql/sem/builtins/math_builtins.go index be9267ae97d2..e390731eda95 100644 --- a/pkg/sql/sem/builtins/math_builtins.go +++ b/pkg/sql/sem/builtins/math_builtins.go @@ -36,6 +36,8 @@ var ( errAbsOfMinInt64 = pgerror.New(pgcode.NumericValueOutOfRange, "abs of min integer value (-9223372036854775808) not defined") errLogOfNegNumber = pgerror.New(pgcode.InvalidArgumentForLogarithm, "cannot take logarithm of a negative number") errLogOfZero = pgerror.New(pgcode.InvalidArgumentForLogarithm, "cannot take logarithm of zero") + + bigTen = apd.NewBigInt(10) ) const ( @@ -517,10 +519,46 @@ var mathBuiltins = map[string]builtinDefinition{ return tree.NewDFloat(tree.DFloat(math.Trunc(x))), nil }, "Truncates the decimal values of `val`.", volatility.Immutable), decimalOverload1(func(x *apd.Decimal) (tree.Datum, error) { + if x.Form == apd.NaN || x.Form == apd.Infinite { + return &tree.DDecimal{Decimal: *x}, nil + } dd := &tree.DDecimal{} x.Modf(&dd.Decimal, nil) return dd, nil }, "Truncates the decimal values of `val`.", volatility.Immutable), + tree.Overload{ + Types: tree.ArgTypes{{"val", types.Decimal}, {"scale", types.Int}}, + ReturnType: tree.FixedReturnType(types.Decimal), + Fn: func(ctx *eval.Context, args tree.Datums) (tree.Datum, error) { + // The algorithm here is also used in shopspring/decimal; see + // https://github.com/shopspring/decimal/blob/f55dd564545cec84cf84f7a53fb3025cdbec1c4f/decimal.go#L1315 + dec := tree.MustBeDDecimal(args[0]).Decimal + scale := -int64(tree.MustBeDInt(args[1])) + if scale > int64(tree.DecimalCtx.MaxExponent) { + scale = int64(tree.DecimalCtx.MaxExponent) + } else if scale < int64(tree.DecimalCtx.MinExponent) { + scale = int64(tree.DecimalCtx.MinExponent) + } + if dec.Form == apd.NaN || dec.Form == apd.Infinite || scale == int64(dec.Exponent) { + return &tree.DDecimal{Decimal: dec}, nil + } else if scale >= (dec.NumDigits() + int64(dec.Exponent)) { + return &tree.DDecimal{Decimal: *decimalZero}, nil + } + ret := &tree.DDecimal{} + diff := math.Abs(float64(scale) - float64(dec.Exponent)) + expScale := apd.NewBigInt(0).Exp(bigTen, apd.NewBigInt(int64(diff)), nil) + if scale > int64(dec.Exponent) { + _ = ret.Coeff.Quo(&dec.Coeff, expScale) + } else if scale < int64(dec.Exponent) { + _ = ret.Coeff.Mul(&dec.Coeff, expScale) + } + ret.Exponent = int32(scale) + ret.Negative = dec.Negative + return ret, nil + }, + Info: "Truncate `val` to `scale` decimal places", + Volatility: volatility.Immutable, + }, ), "width_bucket": makeBuiltin(defProps(),