Merge #144915 #145017 #145106 #145107 #145332 #145367 #145373

144915: sql: disallow set-returning PL/pgSQL functions in mixed version state r=michae2 a=DrewKimball

This commit adds a version check during routine creation for set-returning PL/pgSQL function.

Fixes #144914

Release note (bug fix): Fixed a bug in alpha and beta versions of 25.2 that allowed a set-returning PL/pgSQL function to be created before the version change was finalized.

145017: opt: handle unsimplified groupby orderings r=rytaft a=DrewKimball

This commit fixes a test-only assertion failure due to the GroupBy provided ordering logic relying on ordering simplification rules. The assertion errors happened because the provided ordering logic expected that a required ordering would only ever contain grouping columns. In reality, it is possible for the ordering to include `ConstAgg` and other "pass-through" columns. However, references to these columns are usually removed by rules like `SimplifyGroupByOrdering` because they are constant within a given group.

Since the failure only occurs with rules disabled and only in test builds, there is no release note.

Fixes #137508

Release note: None

145106: roachprod: add CLI option to download certs dir r=srosenberg a=DarrylWong

This change adds a fetch-certs command that will download the pgurl certs from a cluster locally. We need a separate command for certs as opposed to just using roachprod get as lib/pq will complain about world-readable files.

One benefactor of this command is testing out new roachtest operations locally. Many operations require certs and having to manually remember to make them non world readable each time is non obvious and cumbersome.

Release Note: none
Epic: none

145107: schemachanger: check dependents when dropping hash-sharded index r=rafiss a=rafiss

fixes #145100
Release note (bug fix): Fixed a bug where DROP INDEX on a hash-sharded index did not properly detect dependencies from functions and procedures on the shard column. This bug would cause the DROP INDEX statement to fail with an internal validation error. Now, the statement returns a correct error message, and using DROP INDEX ... CASCADE works as expected by dropping the dependent function/procedure.

145332: upgrades: swallow setting override error in diagnostics migration r=rafiss a=msbutler

Previously if a tenant ran the optInToDiagnosticsStatReporting migration, which sets diagnostics.reporting.enabled, after the system tenant already overrode this setting, the migration would enter a fail loop. With this patch, the migration instead noops.

Epic: none

Release note: none

145367: sql: reduce allocations for unsupported type checker r=mgartner a=mgartner

Release note: None

145373: vecindex: bump test size under `race` r=michae2 a=rickystewart

Closes #145299

Epic: none
Release note: None

Co-authored-by: Drew Kimball <[email protected]>
Co-authored-by: DarrylWong <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

Author: 7 people

pkg/ccl/logictestccl/testdata/logic_test/plpgsql_srf

@@ -1,3 +1,5 @@
+# LogicTest: !local-mixed-24.3 !local-mixed-25.1
+
 statement ok
 CREATE TABLE xy (x INT, y INT);
 INSERT INTO xy VALUES (1, 2), (3, 4), (5, 6);

pkg/ccl/logictestccl/testdata/logic_test/tenant_settings

@@ -109,10 +109,10 @@ SHOW CLUSTER SETTING sql.notices.enabled
 true
 
 # Verify that we disallow setting a ApplicationLevel setting that is overridden.
-statement error cluster setting 'sql.notices.enabled' is currently overridden by the operator
+statement error cluster setting 'sql.notices.enabled' cannot be set: cluster setting is overridden by system virtual cluster
 SET CLUSTER SETTING sql.notices.enabled = false
 
-statement error cluster setting 'sql.notices.enabled' is currently overridden by the operator
+statement error cluster setting 'sql.notices.enabled' cannot be set: cluster setting is overridden by system virtual cluster
 RESET CLUSTER SETTING sql.notices.enabled
 
 user host-cluster-root

pkg/ccl/logictestccl/tests/local-mixed-24.3/BUILD.bazel

@@ -9,7 +9,7 @@ go_test(
         "//pkg/ccl/logictestccl:testdata",  # keep
     ],
     exec_properties = {"test.Pool": "large"},
-    shard_count = 33,
+    shard_count = 32,
     tags = ["cpu:1"],
     deps = [
         "//pkg/base",

pkg/ccl/logictestccl/tests/local-mixed-24.3/generated_test.go

@@ -9,7 +9,7 @@ go_test(
         "//pkg/ccl/logictestccl:testdata",  # keep
     ],
     exec_properties = {"test.Pool": "large"},
-    shard_count = 33,
+    shard_count = 32,
     tags = ["cpu:1"],
     deps = [
         "//pkg/base",

pkg/ccl/logictestccl/tests/local-mixed-25.1/BUILD.bazel

@@ -2207,3 +2207,31 @@ If the time is not provided, it downloads the latest pprof file across all clust
 		}),
 	}
 }
+
+func (cr *commandRegistry) buildFetchCertsDir() *cobra.Command {
+	return &cobra.Command{
+		Use:   "fetch-certs <cluster> [<dest-dir>]",
+		Short: "downloads the PGUrl certs directory from the cluster",
+		Long: fmt.Sprintf(`
+Downloads the PGUrl certs directory from the cluster. In addition to downloading the
+certs, it also makes sure the files are not world readable so lib/pq doesn't complain.
+If a destination is not provided, the certs will be downloaded to a default %s directory.
+
+--certs-dir: specify the directory to download the certs from
+
+`, install.CockroachNodeCertsDir),
+		Args: cobra.RangeArgs(1, 2),
+		Run: wrap(func(cmd *cobra.Command, args []string) error {
+			cluster := args[0]
+			// If a destination is not provided, download the certs to a default directory
+			// for safety. FetchCertsDir will walk the entire directory and chmod each file
+			// so we want to avoid side effects.
+			dest := fmt.Sprintf("./%s", install.CockroachNodeCertsDir)
+			if len(args) == 2 {
+				dest = args[1]
+			}
+			ctx := context.Background()
+			return roachprod.FetchCertsDir(ctx, config.Logger, cluster, pgurlCertsDir, dest)
+		}),
+	}
+}

pkg/ccl/logictestccl/tests/local-mixed-25.1/generated_test.go

@@ -70,5 +70,6 @@ func (cr *commandRegistry) register() {
 		cr.buildOpentelemetryStopCmd(),
 		cr.buildFetchLogsCmd(),
 		cr.buildGetLatestPProfCmd(),
+		cr.buildFetchCertsDir(),
 	})
 }

pkg/cmd/roachprod/cli/commands.go

@@ -13,7 +13,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"io/fs"
 	"log"
 	"math/rand"
 	"net"
@@ -2203,6 +2202,7 @@ func (c *clusterImpl) StartE(
 	// Do not refetch certs if that step already happened once (i.e., we
 	// are restarting a node).
 	if settings.Secure && c.localCertsDir == "" {
+		// Get the certs from the first node.
 		if err := c.RefetchCertsFromNode(ctx, 1); err != nil {
 			return err
 		}
@@ -2279,6 +2279,7 @@ func (c *clusterImpl) StartServiceForVirtualClusterE(
 	}
 
 	if settings.Secure {
+		// Get the certs from the first node.
 		if err := c.RefetchCertsFromNode(ctx, 1); err != nil {
 			return err
 		}
@@ -2336,20 +2337,7 @@ func (c *clusterImpl) RefetchCertsFromNode(ctx context.Context, node int) error
 	// certs. Bypass that distinction (which should be fixed independently, but
 	// that might cause fallout) by using a non-existing dir here.
 	c.localCertsDir = filepath.Join(c.localCertsDir, install.CockroachNodeCertsDir)
-	// Get the certs from the first node.
-	if err := c.Get(ctx, c.l, fmt.Sprintf("./%s", install.CockroachNodeCertsDir), c.localCertsDir, c.Node(node)); err != nil {
-		return errors.Wrap(err, "cluster.StartE")
-	}
-	// Need to prevent world readable files or lib/pq will complain.
-	return filepath.WalkDir(c.localCertsDir, func(path string, d fs.DirEntry, err error) error {
-		if err != nil {
-			return errors.Wrap(err, "walking localCertsDir failed")
-		}
-		if d.IsDir() {
-			return nil
-		}
-		return os.Chmod(path, 0600)
-	})
+	return roachprod.FetchCertsDir(ctx, c.l, c.MakeNodes(c.Node(node)), fmt.Sprintf("./%s", install.CockroachNodeCertsDir), c.localCertsDir)
 }
 
 func (c *clusterImpl) SetDefaultVirtualCluster(name string) {

pkg/cmd/roachprod/cli/resgistry.go

@@ -43,14 +43,19 @@ func TestStandbyReadTSPollerJob(t *testing.T) {
 	jobutils.WaitForJobToRun(c.T, c.DestSysSQL, jobspb.JobID(ingestionJobID))
 	t.Logf("test setup took %s", timeutil.Since(beginTS))
 
+	readerTenantName := fmt.Sprintf("%s-readonly", args.DestTenantName)
+
+	// Ensures the reader tenant can spin up even if the system tenant overrode
+	// the diagnostics setting, set during a permanent migration.
+	c.DestSysSQL.Exec(t, fmt.Sprintf("ALTER TENANT '%s' SET CLUSTER SETTING diagnostics.reporting.enabled = true", readerTenantName))
+
 	srcTime := c.SrcCluster.Server(0).Clock().Now()
 	c.WaitUntilReplicatedTime(srcTime, jobspb.JobID(ingestionJobID))
 
 	stats := replicationtestutils.TestingGetStreamIngestionStatsFromReplicationJob(t, ctx, c.DestSysSQL, ingestionJobID)
 	readerTenantID := stats.IngestionDetails.ReadTenantID
 	require.NotNil(t, readerTenantID)
 
-	readerTenantName := fmt.Sprintf("%s-readonly", args.DestTenantName)
 	c.ConnectToReaderTenant(ctx, readerTenantID, readerTenantName)
 
 	defaultDBQuery := `