Merge #28159 #28339 #28394

28159: opt: add support for numeric references r=madhavsuresh a=madhavsuresh

This commit adds support in the optimizer for the following
type of query:

`SELECT * FROM [53 as t]`

Release note:
- Numeric references in opt/ do not use the table descriptor
  cache. This diverges from how numeric references were resolved in the
  heuristic planner.
- This PR changes the semantics of SELECT * FROM [53() as t].
  Previously, and empty column list to a table reference query
  was accepted, and an zero column row was returned. With this
  change, an empty column list is no longer accepted. Empty column
  lists now returns the error: "An explicit list of column IDs must
  include at least one column"

Relevant comment by @knz on this change:

 "I am not 100% sure how to solve this but I would be comfortable to
 'solve' this by rejecting the notation NN() during planning and say with
 an error "an explicit list of column IDs must include at least one
 column".
 Even in the envisioned case for numeric references to handle view
 descriptors, this would be adequate (a numeric reference inside a view
 descriptor will always include at least the PK columns, even if only
 hidden)"

28339: storage: rename ConsistencyServer to StoreMetaServer r=nvanbenschoten,bdarnell a=benesch

The ConsistencyServer will soon learn to assist with merges.
Specifically, it will gain a WaitForAppliedIndex RPC (or something
similar) to block until the specified replica reaches the requested
AppliedIndex.

As such, it needs a more general name. Rename it to StoreMetaServer.

The alternative of creating a separate server for the new RPC seemed
like overkill.

Release note: None

28394: storageccl: improve support for non-S3 endpoints r=mjibson a=mjibson

Remove the requirement for a region if an endpoint is specified by using
a bogus default region string.

If an endpoint is specified, use the old style S3 urls
(http://endpoint/bucket instead of http://bucket.endpoint/).

These two changes were tested with Minio and DO Spaces and both now work
as expected (without specifying a region and no weird endpoint URL).

Fixes #24224
Fixes #21412

Release note (sql change): Improve support for S3-compatible endpoints
in BACKUP, RESTORE, and IMPORT. The AWS_REGION parameter is no longer
required. Services like Digital Ocean Spaces and Minio now work correctly.

Co-authored-by: madhavsuresh <[email protected]>
Co-authored-by: Nikhil Benesch <[email protected]>
Co-authored-by: Matt Jibson <[email protected]>

pkg/ccl/storageccl/export_storage.go

@@ -529,7 +529,7 @@ func makeS3Storage(
 	if conf.Endpoint != "" {
 		config.Endpoint = &conf.Endpoint
 		if conf.Region == "" {
-			return nil, errors.New("s3 region must be specified when using custom endpoints")
+			region = "default-region"
 		}
 		client, err := makeHTTPClient(settings)
 		if err != nil {
@@ -552,6 +552,9 @@ func makeS3Storage(
 		}
 	}
 	sess.Config.Region = aws.String(region)
+	if conf.Endpoint != "" {
+		sess.Config.S3ForcePathStyle = aws.Bool(true)
+	}
 	return &s3Storage{
 		bucket:   aws.String(conf.Bucket),
 		conf:     conf,

pkg/server/node.go

@@ -161,7 +161,7 @@ type Node struct {
 	initialBoot bool // True if this is the first time this node has started.
 	txnMetrics  kv.TxnMetrics
 
-	storesServer storage.Server
+	perReplicaServer storage.Server
 }
 
 // allocateNodeID increments the node id generator key to allocate
@@ -330,7 +330,7 @@ func NewNode(
 		eventLogger: eventLogger,
 		clusterID:   clusterID,
 	}
-	n.storesServer = storage.MakeServer(&n.Descriptor, n.stores)
+	n.perReplicaServer = storage.MakeServer(&n.Descriptor, n.stores)
 	return n
 }
 

pkg/server/server.go

@@ -448,7 +448,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 		storeCfg, s.recorder, s.registry, s.stopper,
 		txnMetrics, nil /* execCfg */, &s.rpcContext.ClusterID)
 	roachpb.RegisterInternalServer(s.grpc, s.node)
-	storage.RegisterConsistencyServer(s.grpc, s.node.storesServer)
+	storage.RegisterPerReplicaServer(s.grpc, s.node.perReplicaServer)
 	s.node.storeCfg.ClosedTimestamp.RegisterClosedTimestampServer(s.grpc)
 
 	s.sessionRegistry = sql.MakeSessionRegistry()

pkg/sql/data_source.go

@@ -226,6 +226,11 @@ func (p *planner) getTableScanByRef(
 		return planDataSource{}, errors.Wrapf(err, "%s", tree.ErrString(tref))
 	}
 
+	if tref.Columns != nil && len(tref.Columns) == 0 {
+		return planDataSource{}, pgerror.NewErrorf(pgerror.CodeSyntaxError,
+			"an explicit list of column IDs must include at least one column")
+	}
+
 	// Ideally, we'd like to populate DatabaseName here, however that
 	// would require a reverse-lookup from DB ID to database name, and
 	// we do not provide an API to do this without a KV lookup. The

pkg/sql/logictest/testdata/logic_test/drop_table

@@ -33,6 +33,9 @@ b
 statement error pgcode 42P01 relation "a" does not exist
 SELECT * FROM a
 
+statement error pq: \[53 AS a\]: table is being dropped
+SELECT * FROM [53 AS a]
+
 statement error pgcode 42P01 relation "a" does not exist
 DROP TABLE a
 

pkg/sql/logictest/testdata/logic_test/numeric_references

@@ -0,0 +1,85 @@
+# LogicTest: local-opt
+# ------------------------------------------------------------------------------
+# Numeric References Tests.
+# These are put at the beginning of the file to ensure the numeric table
+# reference is 53 (the numeric reference of the first table).
+# If the numbering scheme in cockroach changes, this test will break.
+# TODO(madhavsuresh): get the numeric reference ID in a less brittle fashion
+# ------------------------------------------------------------------------------
+
+statement ok
+CREATE TABLE num_ref (a INT PRIMARY KEY, xx INT, b INT, c INT, INDEX bc (b,c))
+
+statement ok
+CREATE TABLE num_ref_hidden (a INT, b INT)
+
+
+statement ok
+ALTER TABLE num_ref RENAME COLUMN b TO d
+
+statement ok
+ALTER TABLE num_ref RENAME COLUMN a TO p
+
+statement ok
+ALTER TABLE num_ref DROP COLUMN xx
+
+statement ok
+INSERT INTO num_ref VALUES (1, 10, 101), (2, 20, 200), (3, 30, 300)
+
+statement ok
+INSERT INTO num_ref_hidden VALUES (1, 10), (2, 20), (3, 30)
+
+query III
+SELECT * FROM num_ref
+----
+1  10  101
+2  20  200
+3  30  300
+
+query III
+SELECT * FROM [53 as num_ref_alias]
+----
+1  10  101
+2  20  200
+3  30  300
+
+query III
+SELECT * FROM [53(4,3,1) AS num_ref_alias]
+----
+101  10  1
+200  20  2
+300  30  3
+
+query I
+SELECT * FROM [53(4) AS num_ref_alias]@[2]
+----
+101
+200
+300
+
+query I
+SELECT * FROM [53(1) AS num_ref_alias]@[1]
+----
+1
+2
+3
+
+query III colnames
+SELECT * FROM [53(1,3,4) AS num_ref_alias(col1,col2,col3)]
+----
+col1  col2  col3
+1     10    101
+2     20    200
+3     30    300
+
+query I
+SELECT * FROM [54(1,3) AS num_ref_hidden]
+----
+1
+2
+3
+
+query I
+SELECT count(rowid) FROM [54(3) AS num_ref_hidden]
+----
+3

pkg/sql/logictest/testdata/logic_test/privileges_table

@@ -11,6 +11,9 @@ SET DATABASE = a
 statement ok
 CREATE TABLE t (k INT PRIMARY KEY, v int)
 
+statement ok
+SELECT * from [54 as num_ref]
+
 statement ok
 SHOW GRANTS ON t
 
@@ -66,6 +69,9 @@ SHOW COLUMNS FROM t
 statement error pq: user testuser does not have SELECT privilege on relation t
 SELECT r FROM t
 
+statement error pq: user testuser does not have SELECT privilege on relation t
+SELECT * from [56 as num_ref]
+
 statement error user testuser does not have GRANT privilege on relation t
 GRANT ALL ON t TO bar
 

pkg/sql/opt/catalog.go

@@ -195,6 +195,11 @@ type Table interface {
 	// position within the table, where i < ColumnCount.
 	Column(i int) Column
 
+	// LookupColumnOrdinal returns the ordinal of the column with the given ID.
+	// Note that this takes the internal column ID, and has no relation to
+	// ColumnIDs in the optimizer.
+	LookupColumnOrdinal(colID uint32) (int, error)
+
 	// IndexCount returns the number of indexes defined on this table. This
 	// includes the primary index, so the count is always >= 1.
 	IndexCount() int
@@ -219,6 +224,10 @@ type Catalog interface {
 	// FindTable returns a Table interface for the database table matching the
 	// given table name.  Returns an error if the table does not exist.
 	FindTable(ctx context.Context, name *tree.TableName) (Table, error)
+
+	// FindTableByID returns a Table interface for the database table
+	// matching the given table ID. Returns an error if the table does not exist.
+	FindTableByID(ctx context.Context, tableID int64) (Table, error)
 }
 
 // FormatCatalogTable nicely formats a catalog table using a treeprinter for