Merge #70215 #70338 #70673

70215: serverccl: Add unit test for unique row id in MT. r=abarganier a=rimadeodhar

This PR adds a unit test to confirm unique row id generates
unique identifiers based on the SQL instance ID in the
multi-tenant environment.

Release note: None

70338: sql: add unordered unique row id as builtin and serial_normalization option r=jameswsj10 a=jameswsj10

See [#54675](#54675). Previously, there were only ordered unique
row ids generated which incurs write hot-spots.
    
The introduction of unordered unique row ids enable distributed
row ids to avoid hotspots while maintaining the 64-bit integer
data type. Basic testing proving distribution of row insertions provided.
    
Release note (sql change): Added a new sql builtin 'unordered_unique_rowid' 
and serial_normalization case 'unordered_rowid' which generates a globally 
unique 64-bit integer that does not have ordering.

70673: cli: fix the process exit status codes r=jbowens a=knz

Found by @jbowens in #70637.
We'll want to backport this. cc @joshimhoff 

Release note (bug fix): The exit status of the `cockroach` command
did not follow the previously-documented table of exit status codes
when an error occurred during the command start-up. (Only errors
occuring after startup were reported using the correct code.)
This defect has been fixed. This bug had existed ever since reference
exit status codes had been introduced.

Co-authored-by: rimadeodhar <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: jameswsj10 <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>

docs/generated/settings/settings-for-tenants.txt

@@ -109,7 +109,7 @@ sql.defaults.prefer_lookup_joins_for_fks.enabled	boolean	false	default value for
 sql.defaults.primary_region	string		if not empty, all databases created without a PRIMARY REGION will implicitly have the given PRIMARY REGION
 sql.defaults.require_explicit_primary_keys.enabled	boolean	false	default value for requiring explicit primary keys in CREATE TABLE statements
 sql.defaults.results_buffer.size	byte size	16 KiB	default size of the buffer that accumulates results for a statement or a batch of statements before they are sent to the client. This can be overridden on an individual connection with the 'results_buffer_size' parameter. Note that auto-retries generally only happen while no results have been delivered to the client, so reducing this size can increase the number of retriable errors a client receives. On the other hand, increasing the buffer size can increase the delay until the client receives the first result row. Updating the setting only affects new connections. Setting to 0 disables any buffering.
-sql.defaults.serial_normalization	enumeration	rowid	default handling of SERIAL in table definitions [rowid = 0, virtual_sequence = 1, sql_sequence = 2, sql_sequence_cached = 3]
+sql.defaults.serial_normalization	enumeration	rowid	default handling of SERIAL in table definitions [rowid = 0, unordered_rowid = 1, virtual_sequence = 2, sql_sequence = 3, sql_sequence_cached = 4]
 sql.defaults.statement_timeout	duration	0s	default value for the statement_timeout; default value for the statement_timeout session setting; controls the duration a query is permitted to run before it is canceled; if set to 0, there is no timeout
 sql.defaults.stub_catalog_tables.enabled	boolean	true	default value for stub_catalog_tables session setting
 sql.defaults.transaction_rows_read_err	integer	0	the limit for the number of rows read by a SQL transaction which - once exceeded - will fail the transaction (or will trigger a logging event to SQL_INTERNAL_PERF for internal transactions); use 0 to disable

docs/generated/settings/settings.html

@@ -114,7 +114,7 @@
 <tr><td><code>sql.defaults.primary_region</code></td><td>string</td><td><code></code></td><td>if not empty, all databases created without a PRIMARY REGION will implicitly have the given PRIMARY REGION</td></tr>
 <tr><td><code>sql.defaults.require_explicit_primary_keys.enabled</code></td><td>boolean</td><td><code>false</code></td><td>default value for requiring explicit primary keys in CREATE TABLE statements</td></tr>
 <tr><td><code>sql.defaults.results_buffer.size</code></td><td>byte size</td><td><code>16 KiB</code></td><td>default size of the buffer that accumulates results for a statement or a batch of statements before they are sent to the client. This can be overridden on an individual connection with the 'results_buffer_size' parameter. Note that auto-retries generally only happen while no results have been delivered to the client, so reducing this size can increase the number of retriable errors a client receives. On the other hand, increasing the buffer size can increase the delay until the client receives the first result row. Updating the setting only affects new connections. Setting to 0 disables any buffering.</td></tr>
-<tr><td><code>sql.defaults.serial_normalization</code></td><td>enumeration</td><td><code>rowid</code></td><td>default handling of SERIAL in table definitions [rowid = 0, virtual_sequence = 1, sql_sequence = 2, sql_sequence_cached = 3]</td></tr>
+<tr><td><code>sql.defaults.serial_normalization</code></td><td>enumeration</td><td><code>rowid</code></td><td>default handling of SERIAL in table definitions [rowid = 0, unordered_rowid = 1, virtual_sequence = 2, sql_sequence = 3, sql_sequence_cached = 4]</td></tr>
 <tr><td><code>sql.defaults.statement_timeout</code></td><td>duration</td><td><code>0s</code></td><td>default value for the statement_timeout; default value for the statement_timeout session setting; controls the duration a query is permitted to run before it is canceled; if set to 0, there is no timeout</td></tr>
 <tr><td><code>sql.defaults.stub_catalog_tables.enabled</code></td><td>boolean</td><td><code>true</code></td><td>default value for stub_catalog_tables session setting</td></tr>
 <tr><td><code>sql.defaults.transaction_rows_read_err</code></td><td>integer</td><td><code>0</code></td><td>the limit for the number of rows read by a SQL transaction which - once exceeded - will fail the transaction (or will trigger a logging event to SQL_INTERNAL_PERF for internal transactions); use 0 to disable</td></tr>

docs/generated/sql/functions.md

@@ -785,6 +785,8 @@ available replica will error.</p>
 </span></td></tr>
 <tr><td><a name="unique_rowid"></a><code>unique_rowid() &rarr; <a href="int.html">int</a></code></td><td><span class="funcdesc"><p>Returns a unique ID used by CockroachDB to generate unique row IDs if a Primary Key isn’t defined for the table. The value is a combination of the insert timestamp and the ID of the node executing the statement, which guarantees this combination is globally unique. However, there can be gaps and the order is not completely guaranteed.</p>
 </span></td></tr>
+<tr><td><a name="unordered_unique_rowid"></a><code>unordered_unique_rowid() &rarr; <a href="int.html">int</a></code></td><td><span class="funcdesc"><p>Returns a unique ID. The value is a combination of the insert timestamp and the ID of the node executing the statement, which guarantees this combination is globally unique. The way it is generated there is no ordering</p>
+</span></td></tr>
 <tr><td><a name="uuid_generate_v4"></a><code>uuid_generate_v4() &rarr; <a href="uuid.html">uuid</a></code></td><td><span class="funcdesc"><p>Generates a random UUID and returns it as a value of UUID type.</p>
 </span></td></tr>
 <tr><td><a name="uuid_v4"></a><code>uuid_v4() &rarr; <a href="bytes.html">bytes</a></code></td><td><span class="funcdesc"><p>Returns a UUID.</p>

pkg/ccl/serverccl/server_sql_test.go

@@ -11,6 +11,7 @@ package serverccl
 import (
 	"context"
 	"errors"
+	"fmt"
 	"io/ioutil"
 	"net/http"
 	"testing"
@@ -186,3 +187,40 @@ func TestNonExistentTenant(t *testing.T) {
 	require.Error(t, err)
 	require.Equal(t, "system DB uninitialized, check if tenant is non existent", err.Error())
 }
+
+// TestTenantRowIDs confirms `unique_rowid()` works as expected in a
+// multi-tenant setup.
+func TestTenantRowIDs(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+	ctx := context.Background()
+
+	tc := serverutils.StartNewTestCluster(t, 3, base.TestClusterArgs{})
+	defer tc.Stopper().Stop(ctx)
+	const numRows = 10
+	tenant, db := serverutils.StartTenant(
+		t,
+		tc.Server(0),
+		base.TestTenantArgs{TenantID: serverutils.TestTenantID()},
+	)
+	defer db.Close()
+	sqlDB := sqlutils.MakeSQLRunner(db)
+	sqlDB.Exec(t, `CREATE TABLE foo(key INT PRIMARY KEY DEFAULT unique_rowid(), val INT)`)
+	sqlDB.Exec(t, fmt.Sprintf("INSERT INTO foo (val) SELECT * FROM generate_series(1, %d)", numRows))
+
+	// Verify that the rows are inserted successfully and that the row ids
+	// are based on the SQL instance ID.
+	rows := sqlDB.Query(t, "SELECT key FROM foo")
+	defer rows.Close()
+	rowCount := 0
+	instanceID := int(tenant.SQLInstanceID())
+	for rows.Next() {
+		var key int
+		if err := rows.Scan(&key); err != nil {
+			t.Fatal(err)
+		}
+		require.Equal(t, instanceID, key&instanceID)
+		rowCount++
+	}
+	require.Equal(t, numRows, rowCount)
+}

pkg/cli/BUILD.bazel

@@ -311,11 +311,14 @@ go_test(
         "//pkg/base",
         "//pkg/build",
         "//pkg/cli/clicfg",
+        "//pkg/cli/clierror",
+        "//pkg/cli/clierrorplus",
         "//pkg/cli/cliflags",
         "//pkg/cli/clisqlcfg",
         "//pkg/cli/clisqlclient",
         "//pkg/cli/clisqlexec",
         "//pkg/cli/democluster",
+        "//pkg/cli/exit",
         "//pkg/gossip",
         "//pkg/jobs",
         "//pkg/jobs/jobspb",

pkg/cli/cli.go

@@ -67,16 +67,21 @@ func Main() {
 
 		// Finally, extract the error code, as optionally specified
 		// by the sub-command.
-		errCode = exit.UnspecifiedError()
-		var cliErr *clierror.Error
-		if errors.As(err, &cliErr) {
-			errCode = cliErr.GetExitCode()
-		}
+		errCode = getExitCode(err)
 	}
 
 	exit.WithCode(errCode)
 }
 
+func getExitCode(err error) (errCode exit.Code) {
+	errCode = exit.UnspecifiedError()
+	var cliErr *clierror.Error
+	if errors.As(err, &cliErr) {
+		errCode = cliErr.GetExitCode()
+	}
+	return errCode
+}
+
 func doMain(cmd *cobra.Command, cmdName string) error {
 	if cmd != nil {
 		// Apply the configuration defaults from environment variables.

pkg/cli/cli_test.go

@@ -14,9 +14,13 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/cockroachdb/cockroach/pkg/cli/clierror"
+	"github.com/cockroachdb/cockroach/pkg/cli/clierrorplus"
+	"github.com/cockroachdb/cockroach/pkg/cli/exit"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/errors"
+	"github.com/spf13/cobra"
 )
 
 func TestCLITimeout(t *testing.T) {
@@ -72,3 +76,26 @@ func TestJunkPositionalArguments(t *testing.T) {
 		}
 	}
 }
+
+func Example_exitcode() {
+	c := NewCLITest(TestCLIParams{NoServer: true})
+	defer c.Cleanup()
+
+	testCmd := &cobra.Command{
+		Use: "test-exit-code",
+		RunE: clierrorplus.MaybeDecorateError(
+			func(_ *cobra.Command, _ []string) error {
+				return clierror.NewError(errors.New("err"), exit.Interrupted())
+			}),
+	}
+	cockroachCmd.AddCommand(testCmd)
+	defer cockroachCmd.RemoveCommand(testCmd)
+
+	c.reportExitCode = true
+	c.Run("test-exit-code")
+
+	// Output:
+	// test-exit-code
+	// ERROR: err
+	// exit code: 3
+}

pkg/cli/clierror/formatted_error.go

@@ -57,6 +57,10 @@ type formattedError struct {
 	showSeverity, verbose bool
 }
 
+func (f *formattedError) Unwrap() error {
+	return f.err
+}
+
 // Error implements the error interface.
 func (f *formattedError) Error() string {
 	// If we're applying recursively, ignore what's there and display the original error.

pkg/cli/interactive_tests/test_server_sig.tcl

@@ -34,9 +34,9 @@ eexpect "shutdown completed"
 eexpect ":/# "
 end_test
 
-start_test "Check that Ctrl+C finishes with exit code 1. (#9051)"
+start_test "Check that Ctrl+C finishes with exit code 3. (#9051+#70673)"
 send "echo \$?\r"
-eexpect "1\r\n"
+eexpect "3\r\n"
 eexpect ":/# "
 end_test
 

pkg/cli/testutils.go

@@ -28,6 +28,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cli/clierror"
 	"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
 	"github.com/cockroachdb/cockroach/pkg/cli/clisqlexec"
+	"github.com/cockroachdb/cockroach/pkg/cli/exit"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
@@ -61,15 +62,23 @@ type TestCLI struct {
 	logScope *log.TestLogScope
 	// if true, doesn't print args during RunWithArgs.
 	omitArgs bool
+	// if true, prints the requested exit code during RunWithArgs.
+	reportExitCode bool
 }
 
 // TestCLIParams contains parameters used by TestCLI.
 type TestCLIParams struct {
-	T           *testing.T
-	Insecure    bool
-	NoServer    bool
-	StoreSpecs  []base.StoreSpec
-	Locality    roachpb.Locality
+	T        *testing.T
+	Insecure bool
+	// NoServer, if true, starts the test without a DB server.
+	NoServer bool
+
+	// The store specifications for the in-memory server.
+	StoreSpecs []base.StoreSpec
+	// The locality tiers for the in-memory server.
+	Locality roachpb.Locality
+
+	// NoNodelocal, if true, disables node-local external I/O storage.
 	NoNodelocal bool
 }
 
@@ -350,6 +359,13 @@ func (c TestCLI) RunWithArgs(origArgs []string) {
 		return Run(args)
 	}(); err != nil {
 		clierror.OutputError(os.Stdout, err, true /*showSeverity*/, false /*verbose*/)
+		if c.reportExitCode {
+			fmt.Fprintln(os.Stdout, "exit code:", getExitCode(err))
+		}
+	} else {
+		if c.reportExitCode {
+			fmt.Fprintln(os.Stdout, "exit code:", exit.Success())
+		}
 	}
 }
 

pkg/sql/exec_util.go

@@ -534,6 +534,7 @@ var SerialNormalizationMode = settings.RegisterEnumSetting(
 	"rowid",
 	map[int64]string{
 		int64(sessiondatapb.SerialUsesRowID):              "rowid",
+		int64(sessiondatapb.SerialUsesUnorderedRowID):     "unordered_rowid",
 		int64(sessiondatapb.SerialUsesVirtualSequences):   "virtual_sequence",
 		int64(sessiondatapb.SerialUsesSQLSequences):       "sql_sequence",
 		int64(sessiondatapb.SerialUsesCachedSQLSequences): "sql_sequence_cached",

pkg/sql/sem/builtins/BUILD.bazel

@@ -139,6 +139,7 @@ go_test(
         "//pkg/util/leaktest",
         "//pkg/util/randutil",
         "//pkg/util/timeutil",
+        "@com_github_lib_pq//:pq",
         "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
     ],

pkg/sql/sem/builtins/builtins.go

@@ -25,6 +25,7 @@ import (
 	"hash/fnv"
 	"io/ioutil"
 	"math"
+	"math/bits"
 	"math/rand"
 	"net"
 	"regexp/syntax"
@@ -2013,6 +2014,25 @@ var builtins = map[string]builtinDefinition{
 		},
 	),
 
+	"unordered_unique_rowid": makeBuiltin(
+		tree.FunctionProperties{
+			Category: categoryIDGeneration,
+		},
+		tree.Overload{
+			Types:      tree.ArgTypes{},
+			ReturnType: tree.FixedReturnType(types.Int),
+			Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
+				v := GenerateUniqueUnorderedID(ctx.NodeID.SQLInstanceID())
+				return tree.NewDInt(v), nil
+			},
+			Info: "Returns a unique ID. The value is a combination of the " +
+				"insert timestamp and the ID of the node executing the statement, which " +
+				"guarantees this combination is globally unique. The way it is generated " +
+				"there is no ordering",
+			Volatility: tree.VolatilityVolatile,
+		},
+	),
+
 	// Sequence functions.
 
 	"nextval": makeBuiltin(
@@ -7583,6 +7603,26 @@ func overlay(s, to string, pos, size int) (tree.Datum, error) {
 // GenerateUniqueInt.
 const NodeIDBits = 15
 
+// GenerateUniqueUnorderedID creates a unique int64 composed of the current time
+// at a 10-microsecond granularity and the instance-id. The top-bit is left
+// empty so that negative values are not returned. The 48 bits following after
+// represent the reversed timestamp and then 15 bits of the node id.
+func GenerateUniqueUnorderedID(instanceID base.SQLInstanceID) tree.DInt {
+	orig := uint64(GenerateUniqueInt(instanceID))
+	uniqueUnorderedID := mapToUnorderedUniqueInt(orig)
+	return tree.DInt(uniqueUnorderedID)
+}
+
+// mapToUnorderedUniqueInt is used by GenerateUniqueUnorderedID to convert a
+// serial unique uint64 to an unordered unique int64. The bit manipulation
+// should preserve the number of 1-bits.
+func mapToUnorderedUniqueInt(val uint64) uint64 {
+	// val is [0][48 bits of ts][15 bits of node id]
+	ts := (val & ((uint64(math.MaxUint64) >> 16) << 15)) >> 15
+	v := (bits.Reverse64(ts) >> 1) | (val & (1<<15 - 1))
+	return v
+}
+
 // GenerateUniqueInt creates a unique int composed of the current time at a
 // 10-microsecond granularity and the instance-id. The instance-id is stored in the
 // lower 15 bits of the returned value and the timestamp is stored in the upper