Merge #158708 #158803 #158899

158708: opt: route descriptor lookups for AFTER triggers through cache r=mgartner a=mgartner

#### bench/rttananlysis: document directions for running tests

These tests have a non-conventional construction that make it hard to
determine how they can be run. Each subset of tests is not a typical Go
function like `func TestAThing(t *testing.T) { ... }` that can be easily
searched for. A few comments have been added to explain how the tests
can be run.

Release note: None

#### bench/rttanalysis: add test case with an after trigger

Release note: None

#### opt: route descriptor lookups for AFTER triggers through cache

This commit applies the fix in #144217 for `BEFORE` triggers to `AFTER`
triggers. See that PR for more details.

Fixes #144211

Release note (performance improvement): After triggers now perform the
descriptor lookup for `TG_TABLE_SCHEMA` against a cache. This can
significantly reduce trigger planning latency.

#### opt: warn about performance of (*optCatalog).FullyQualifiedName

Release note: None


158803: gen-cockroachdb-metrics: remove excessive logging r=rail,Abhinav1299 a=rickystewart

This tool is unnecessarily noisy and always prints even when successful. Guard this behavior behind a --verbose command-line flag. Also use the `flag` library for command-line flags.

Release note: none
Epic: none

158899: ci: skip AI review for draft PRs r=rickystewart a=ajstorm

## Summary
- Skip Claude Code PR Review workflow for draft PRs
- Draft PRs are work-in-progress and not ready for review, so running AI review wastes resources
- The workflow will still trigger when a PR is marked ready for review via the `ready_for_review` event

Release note: None
Epic: None

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Adam Storm <[email protected]>

Author: 4 people

.github/workflows/pr-analyzer-threestage.yml

@@ -8,7 +8,7 @@ jobs:
   claude-code-pr-review:
     runs-on: ubuntu-latest
     timeout-minutes: 60
-    if: "!startsWith(github.base_ref, 'release-') && !contains(github.event.pull_request.labels.*.name, 'O-No-AI-Review') && github.event.pull_request.merged == false"
+    if: "!startsWith(github.base_ref, 'release-') && !contains(github.event.pull_request.labels.*.name, 'O-No-AI-Review') && !github.event.pull_request.merged && !github.event.pull_request.draft"
     permissions:
       contents: read
       pull-requests: write

build/tools/gen-cockroachdb-metrics/main.go

@@ -36,6 +36,7 @@
 package main
 
 import (
+	"flag"
 	"fmt"
 	"io"
 	"os"
@@ -106,6 +107,9 @@ var (
 
 	// Regex to convert CRDB metric names to Prometheus format
 	prometheusNameRegex = regexp.MustCompile(`[^a-zA-Z0-9]`)
+
+	datadogOnly = flag.Bool("datadog-only", false, "generate metrics in datadog-only mode")
+	verbose     = flag.Bool("verbose", false, "print diagnostic logs")
 )
 
 // YAML template for generating the cockroachdb_metrics.yaml file
@@ -458,20 +462,19 @@ func printUsage() {
 }
 
 func main() {
-	if len(os.Args) < 2 {
-		printUsage()
-		os.Exit(1)
-	}
+	flag.Parse()
 
+	args := flag.Args()
+	
 	// Check if running in datadog-only mode
-	if os.Args[1] == "--datadog-only" {
-		if len(os.Args) < 4 {
+	if *datadogOnly {
+		if len(flag.Args()) < 2 {
 			printUsage()
 			os.Exit(1)
 		}
 
-		datadogMappingsPath := os.Args[2]
-		outputPath := os.Args[3]
+		datadogMappingsPath := args[0]
+		outputPath := args[1]
 
 		datadogMappings, err := loadDatadogMappings(datadogMappingsPath)
 		if err != nil {
@@ -484,27 +487,27 @@ func main() {
 			os.Exit(1)
 		}
 
-		if !strings.Contains(outputPath, "-exec-") && !strings.Contains(outputPath, "for tool") {
+		if *verbose {
 			fmt.Printf("Generated %d raw Datadog metric mappings\n", len(datadogMappings))
 			fmt.Printf("Output written to: %s\n", outputPath)
 		}
 		return
 	}
 
 	// Normal mode: generate cockroachdb_metrics.yaml
-	if len(os.Args) < 4 {
+	if len(args) < 3 {
 		printUsage()
 		os.Exit(1)
 	}
 
-	metricsYamlPath := os.Args[1]
-	outputPath := os.Args[2]
-	datadogMappingsPath := os.Args[3]
+	metricsYamlPath := args[0]
+	outputPath := args[1]
+	datadogMappingsPath := args[2]
 
 	// Optional fourth argument: base YAML file with extra and legacy metrics
 	basePath := ""
-	if len(os.Args) >= 5 {
-		basePath = os.Args[4]
+	if len(args) >= 4 {
+		basePath = args[3]
 	}
 
 	datadogMappings, err := loadDatadogMappings(datadogMappingsPath)
@@ -534,9 +537,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	// Only print summary if not in exec/tool configuration to avoid duplicate logs.
-	// Bazel builds the tool in both target and exec configurations, causing duplicate output.
-	if !strings.Contains(outputPath, "-exec-") && !strings.Contains(outputPath, "for tool") {
+	if *verbose {
 		fmt.Printf("Generated %d metric mappings:\n", len(finalMappings))
 		fmt.Printf("  - %d from metrics.yaml\n", len(allMetrics)-len(baseMetrics.RuntimeConditionalMetrics))
 		fmt.Printf("  - %d runtime conditional metrics\n", len(baseMetrics.RuntimeConditionalMetrics))

pkg/bench/rttanalysis/registry.go

@@ -24,6 +24,14 @@ import (
 // This structure facilitates a registry concept without needing to rename
 // existing benchmarks.
 //
+// All registered benchmark tests can be run with the command:
+//
+//	./dev test pkg/bench/rttanalysis -f TestBenchmarkExpectation
+//
+// A specific benchmark test can be run by adding its registered name as a suffix to the filter:
+//
+//	./dev test pkg/bench/rttanalysis -f TestBenchmarkExpectation/CreateRole
+//
 // The expectation is that there should be a single registry per package and
 // that tests are registered to it during initialization.
 type Registry struct {

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -119,7 +119,8 @@ exp,benchmark
 1,SystemDatabaseQueries/select_system.users_with_empty_database_Name
 1,SystemDatabaseQueries/select_system.users_with_schema_Name
 1,SystemDatabaseQueries/select_system.users_without_schema_Name
-1,TriggerResolution/insert_into_table_with_trigger
+1,TriggerResolution/insert_into_table_with_before_trigger
+1,TriggerResolution/insert_into_table_with_after_trigger
 18,Truncate/truncate_1_column_0_rows
 17,Truncate/truncate_1_column_1_row
 17,Truncate/truncate_1_column_2_rows

pkg/bench/rttanalysis/trigger_resolution_test.go

@@ -7,18 +7,32 @@ package rttanalysis
 
 import "testing"
 
+// BenchmarkTriggerResolution benchmarks the KV round-trips taken to
+// execute statements with triggers.
+//
+// This benchmark can be run with the command:
+//
+//	./dev test pkg/bench/rttanalysis -f TestBenchmarkExpectation/TriggerResolution
 func BenchmarkTriggerResolution(b *testing.B) {
 	reg.Run(b)
 }
 func init() {
 	reg.Register("TriggerResolution", []RoundTripBenchTestCase{
 		{
-			Name: "insert into table with trigger",
+			Name: "insert into table with before trigger",
 			Setup: `
         CREATE TABLE trigger_table (a INT);
         CREATE FUNCTION trigger_fn() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$ BEGIN RETURN NEW; END $$;
         CREATE TRIGGER tr BEFORE INSERT ON trigger_table FOR EACH ROW EXECUTE FUNCTION trigger_fn();`,
 			Stmt: `INSERT INTO trigger_table VALUES (100);`,
 		},
+		{
+			Name: "insert into table with after trigger",
+			Setup: `
+        CREATE TABLE trigger_table (a INT);
+        CREATE FUNCTION trigger_fn() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$ BEGIN RETURN NEW; END $$;
+        CREATE TRIGGER tr AFTER INSERT ON trigger_table FOR EACH ROW EXECUTE FUNCTION trigger_fn();`,
+			Stmt: `INSERT INTO trigger_table VALUES (100);`,
+		},
 	})
 }

pkg/sql/opt/optbuilder/trigger.go

@@ -660,11 +660,13 @@ func (tb *rowLevelAfterTriggerBuilder) Build(
 			tgLevel := tree.NewDString("ROW")
 			tgRelID := tree.NewDOid(oid.Oid(tb.mutatedTable.ID()))
 			tgTableName := tree.NewDString(string(tb.mutatedTable.Name()))
-			fqName, err := b.catalog.FullyQualifiedName(ctx, tb.mutatedTable)
+			schema, err := b.catalog.ResolveSchemaByID(
+				b.ctx, cat.Flags{}, cat.StableID(tb.mutatedTable.GetSchemaID()),
+			)
 			if err != nil {
 				panic(err)
 			}
-			tgTableSchema := tree.NewDString(fqName.Schema())
+			tgTableSchema := tree.NewDString(schema.Name().Schema())
 			var tgOp opt.ScalarExpr
 			switch tb.mutation {
 			case opt.InsertOp:

pkg/sql/opt_catalog.go

@@ -530,6 +530,12 @@ func (oc *optCatalog) UserHasGlobalPrivilegeOrRoleOption(
 }
 
 // FullyQualifiedName is part of the cat.Catalog interface.
+//
+// Note that:
+//   - this call may involve a database operation so it shouldn't be used in
+//     performance sensitive paths;
+//   - the fully qualified name of a data source object can change without the
+//     object itself changing (e.g. when a database is renamed).
 func (oc *optCatalog) FullyQualifiedName(
 	ctx context.Context, ds cat.DataSource,
 ) (cat.DataSourceName, error) {