Merge #142469 #144051 #144266 #144297

142469: microbench-ci: combine compare and post r=rickystewart a=herkolategan

Previously, the microbench-ci tool had separate commands for comparing benchmark results and posting them to GitHub.

This change combines the compare and post functionality into a single command, simplifying the CI workflow by reducing the number of commands needed. When `--post` is specified, the tool will both generate the comparison summary and conditionally post it as a GitHub comment.

Previously, a single comment was posted or updated on the PR for each invocation of the tool. Now, a new comment will be posted for each invocation of the tool if either the `X-perf-check` label is present or if the benchmark results have significant changes.

Epic: None
Release note: None

144051: raft: forward appended LogSlice to commit index r=tbg a=pav-kv

When handling `MsgApp`, we never overwrite committed entries. Previously, `MsgApp` would be ignored if its `prev.index < raftLog.committed`. This was too strict: the appended slice can still have new entries > `committed` index. This PR makes it possible to accept such appends, after "forwarding" the appended slice to `committed` index. The `match` check still ensures that this partial append is correct.

Epic: none
Release note: none

144266: jsonpath: add support for `.size()`, `.type()` methods r=normanchenn a=normanchenn

#### jsonpath: add support for .size() method

This commit adds support for the `.size()` method in JSONPath queries,
which returns the length of the array or 1 for non-array values (in lax
mode). In strict mode, it returns an error when applied to non-array
values.

This commit also sets up the rest of the JSONPath methods, adding
unimplemented errors when they are parsed.

Informs: #22513
Release note (sql change): Add support for `.size()` method in JSONPath
expressions. For example, `SELECT jsonb_path_query('[1, 2, 3]',
'$.size()');`.

#### jsonpath: add support for .type() method

This commit adds support for the `.type()` method in JSONPath queries,
which returns a string descripbing the type of the current JSON value
("object", "array", "string", "number", "boolean", "null").

Informs: #22513
Release note (sql change): Add support for `.type()` method for JSONPath
queries. For example, `SELECT jsonb_path_query('[1, 2, 3]', '$.type()');`.

144297: sql: avoid unnecessary version bumps for UDTs during ALTER TABLE r=spilchen a=spilchen

Previously, in the legacy schema changer, we unconditionally updated backreferences from tables to user-defined types (UDTs) during ALTER TABLE operations. This happened even when the backreferences remained unchanged.

While this behavior was functionally harmless, it caused the version of the referenced types to be incremented unnecessarily.

This change avoids version bumps unless the backreference actually changes.

Fixes: #144293

Epic: none
Release note: none

Co-authored-by: Herko Lategan <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>
Co-authored-by: Norman Chen <[email protected]>
Co-authored-by: Matt Spilchen <[email protected]>

Author: 5 people

.github/workflows/microbenchmarks-ci.yaml

@@ -19,6 +19,7 @@ jobs:
     name: build merge base
     runs-on: [self-hosted, basic_runner_group]
     timeout-minutes: 30
+    if: ${{ !contains(github.event.pull_request.labels.*.name, 'X-skip-perf-check') }}
     outputs:
       merge_base: ${{ steps.build.outputs.merge_base }}
     steps:
@@ -34,6 +35,7 @@ jobs:
     name: build head
     runs-on: [self-hosted, basic_runner_group]
     timeout-minutes: 30
+    if: ${{ !contains(github.event.pull_request.labels.*.name, 'X-skip-perf-check') }}
     steps:
       - name: Checkout
         uses: actions/checkout@v4
@@ -72,32 +74,11 @@ jobs:
   compare:
     runs-on: [self-hosted, basic_runner_group]
     timeout-minutes: 30
-    needs: [base, run-group-1, run-group-2]
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-      - run: ./build/github/get-engflow-keys.sh
-        shell: bash
-      - name: Unique Build ID
-        run: echo "BUILD_ID=${{ github.run_id }}-${{ github.run_attempt }}" >> $GITHUB_ENV
-      - name: Compare and Post
-        run: ./build/github/microbenchmarks/compare.sh
-        env:
-          BASE_SHA: ${{ needs.base.outputs.merge_base }}
-          HEAD_SHA: ${{ env.HEAD }}
-      - name: Clean up
-        run: ./build/github/cleanup-engflow-keys.sh
-        shell: bash
-        if: always()
-  post:
-    runs-on: [ self-hosted, basic_runner_group ]
-    if: contains(github.event.pull_request.labels.*.name, 'T-microbench-post') # TODO: remove this check once results are confirmed to be stable on CI.
-    timeout-minutes: 30
     permissions:
         contents: read
         pull-requests: write
-        issues: write
-    needs: [ base, compare ]
+        issues: write    
+    needs: [base, run-group-1, run-group-2]
     steps:
       - name: Checkout
         uses: actions/checkout@v4
@@ -106,12 +87,13 @@ jobs:
       - name: Unique Build ID
         run: echo "BUILD_ID=${{ github.run_id }}-${{ github.run_attempt }}" >> $GITHUB_ENV
       - name: Compare and Post
-        run: ./build/github/microbenchmarks/post.sh
+        run: ./build/github/microbenchmarks/compare.sh
         env:
+          BASE_SHA: ${{ needs.base.outputs.merge_base }}
           HEAD_SHA: ${{ env.HEAD }}
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-          REPO: "cockroachdb/cockroach"
-          PR_NUMBER: ${{ github.event.pull_request.number }}
+          GITHUB_REPO: "cockroachdb/cockroach"
+          GITHUB_PR_NUMBER: ${{ github.event.pull_request.number }}
       - name: Clean up
         run: ./build/github/cleanup-engflow-keys.sh
         shell: bash

build/github/microbenchmarks/compare.sh

@@ -28,7 +28,8 @@ done
   --github-summary="$output_dir/summary.md" \
   --build-id="$BUILD_ID" \
   --old="$BASE_SHA" \
-  --new="$HEAD_SHA"
+  --new="$HEAD_SHA" \
+  --post
 
 cat "$output_dir/summary.md" > "$GITHUB_STEP_SUMMARY"
 

build/github/microbenchmarks/post.sh

@@ -1401,7 +1401,7 @@ func createImportingDescriptors(
 					if err != nil {
 						return err
 					}
-					typDesc.AddReferencingDescriptorID(table.GetID())
+					_ = typDesc.AddReferencingDescriptorID(table.GetID())
 					if err := descsCol.WriteDescToBatch(
 						ctx, kvTrace, typDesc, b,
 					); err != nil {
@@ -3119,7 +3119,7 @@ func (r *restoreResumer) removeExistingTypeBackReferences(
 				}
 				existing := desc.(*typedesc.Mutable)
 				existing.MaybeIncrementVersion()
-				existing.RemoveReferencingDescriptorID(tbl.ID)
+				_ = existing.RemoveReferencingDescriptorID(tbl.ID)
 			}
 		}
 	}

pkg/backup/restore_job.go

@@ -6,6 +6,7 @@
 package main
 
 import (
+	"fmt"
 	"os"
 	"os/exec"
 	"path"
@@ -107,13 +108,13 @@ func TestRunAndCompare(t *testing.T) {
 				require.NoError(t, err)
 				results, err := suite.Benchmarks.compareBenchmarks()
 				require.NoError(t, err)
-				err = results.writeGitHubSummary(config.GitHubSummaryPath)
+				summary, err := results.githubSummary()
 				require.NoError(t, err)
 				err = results.writeJSONSummary(config.SummaryPath)
 				require.NoError(t, err)
-				data, err := os.ReadFile(config.GitHubSummaryPath)
-				require.NoError(t, err)
-				return string(data)
+				return summary
+			case "changed":
+				return fmt.Sprintf("%t", suite.changeDetected())
 			case "json":
 				data, err := os.ReadFile(config.SummaryPath)
 				require.NoError(t, err)

pkg/cmd/microbench-ci/compare_test.go

@@ -10,9 +10,9 @@ benchmarks:
     retries: 3
     metrics:
       - name: "sec/op"
-        threshold: .3
+        threshold: .6
       - name: "allocs/op"
-        threshold: .1
+        threshold: .2
 
   - display_name: Sysbench
     labels: ["KV", "3node", "oltp_read_only"]
@@ -25,9 +25,9 @@ benchmarks:
     retries: 3
     metrics:
       - name: "sec/op"
-        threshold: .4
+        threshold: 0.9
       - name: "allocs/op"
-        threshold: .1
+        threshold: .3
 
   - display_name: Sysbench
     labels: ["KV", "3node", "oltp_write_only"]
@@ -40,6 +40,6 @@ benchmarks:
     retries: 3
     metrics:
       - name: "sec/op"
-        threshold: .4
+        threshold: 0.9
       - name: "allocs/op"
-        threshold: .1
+        threshold: .3

pkg/cmd/microbench-ci/config/pull-request-suite.yml

@@ -9,60 +9,102 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"strconv"
 	"strings"
 
 	"github.com/cockroachdb/errors"
 	"github.com/google/go-github/v61/github"
 	"golang.org/x/oauth2"
 )
 
-// CommentTag is used to identify an existing comment.
-const CommentTag = "<!-- microbench-ci -->"
+type GitHubConfig struct {
+	client *github.Client
+	Owner  string
+	Repo   string
+	PrNum  int
+}
+
+const (
+	CommentTag = "<!-- microbench-ci -->"
+	PerfLabel  = "X-perf-check"
+)
 
-// post posts or updates a comment on a GitHub PR, with the given summary text,
-// that has the CommentTag marker in it.
-func post(summaryText, githubRepository string, prNumber int) error {
+// NewGithubConfig creates a new GitHubConfig from the environment variables.
+func NewGithubConfig() (*GitHubConfig, error) {
 	token := os.Getenv("GITHUB_TOKEN")
 	if token == "" {
-		return errors.New("GITHUB_TOKEN is not set, this command is meant to be run in a GitHub Action")
+		return nil, errors.New("GITHUB_TOKEN is not set")
 	}
 
-	repoInfo := strings.Split(githubRepository, "/")
-	if len(repoInfo) != 2 {
-		return errors.New("invalid GitHub repository flag")
-	}
-	owner := repoInfo[0]
-	repo := repoInfo[1]
-
 	ctx := context.Background()
 	ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token})
 	tc := oauth2.NewClient(ctx, ts)
 	client := github.NewClient(tc)
 
-	// Check for an existing comment
-	comments, _, err := client.Issues.ListComments(ctx, owner, repo, prNumber, nil)
+	githubRepository := os.Getenv("GITHUB_REPO")
+	if githubRepository == "" {
+		return nil, errors.New("GITHUB_REPO is not set")
+	}
+	prNumber := os.Getenv("GITHUB_PR_NUMBER")
+	if prNumber == "" {
+		return nil, errors.New("GITHUB_PR_NUMBER is not set")
+	}
+	prNumberInt, err := strconv.Atoi(prNumber)
 	if err != nil {
-		return err
+		return nil, errors.Newf("invalid GITHUB_PR_NUMBER format %s", prNumber)
 	}
-	var existingComment *github.IssueComment
-	for _, comment := range comments {
-		if strings.Contains(comment.GetBody(), CommentTag) {
-			existingComment = comment
-			break
-		}
+	repoInfo := strings.Split(githubRepository, "/")
+	if len(repoInfo) != 2 {
+		return nil, errors.Newf("invalid GitHub repository flag, %s", githubRepository)
 	}
+	owner := repoInfo[0]
+	repo := repoInfo[1]
+
+	return &GitHubConfig{
+		client: client,
+		Owner:  owner,
+		Repo:   repo,
+		PrNum:  prNumberInt,
+	}, nil
+}
 
+// postComment posts a comment on a GitHub PR, with the given summary text, that
+// has the CommentTag marker in it.
+func (c *GitHubConfig) postComment(summaryText string) error {
+	ctx := context.Background()
 	commentBody := github.String(fmt.Sprintf("%s\n%s", summaryText, CommentTag))
-	if existingComment != nil {
-		// Update the existing comment
-		existingComment.Body = commentBody
-		_, _, err = client.Issues.EditComment(ctx, owner, repo, existingComment.GetID(), existingComment)
-		return err
-	}
+	_, _, err := c.client.Issues.CreateComment(
+		ctx, c.Owner, c.Repo, c.PrNum,
+		&github.IssueComment{
+			Body: commentBody,
+		},
+	)
+	return err
+}
 
-	// Create a new comment
-	_, _, err = client.Issues.CreateComment(ctx, owner, repo, prNumber, &github.IssueComment{
-		Body: commentBody,
-	})
+// addLabel adds a label to a GitHub PR.
+func (c *GitHubConfig) addLabel(label string) error {
+	ctx := context.Background()
+	_, _, err := c.client.Issues.AddLabelsToIssue(
+		ctx, c.Owner, c.Repo, c.PrNum,
+		[]string{label},
+	)
 	return err
 }
+
+// hasLabel checks if a GitHub PR has a label.
+func (c *GitHubConfig) hasLabel(label string) (bool, error) {
+	ctx := context.Background()
+	labels, _, err := c.client.Issues.ListLabelsByIssue(
+		ctx, c.Owner, c.Repo, c.PrNum, &github.ListOptions{PerPage: 100},
+	)
+	if err != nil {
+		return false, err
+	}
+	for _, l := range labels {
+		if *l.Name == label {
+			return true, nil
+		}
+	}
+	return false, nil
+}