Merge #82873 #83282

82873: storage: support MVCC range tombstones in `MVCCExportToSST` r=itsbilal a=erikgrinaker

This patch adds support for exporting MVCC range tombstones in
`MVCCExportToSST()`, and by extension in the KV `Export` method.
This will only happen after two version gates are enabled:

* `EnablePebbleFormatVersionRangeKeys`: begins emitting SSTs in
  `Pebblev2` format, which supports Pebble range keys.

* `MVCCRangeTombstones`: allows writing MVCC range tombstones via KV.

MVCC range tombstones are emitted in the same way as point tombstones:
all tombstones if `ExportAllRevisions` is enabled, or the latest visible
tombstone if `StartTS` is given.

MVCC range tombstones are truncated to the SST bounds. For example, if
exporting the span `a-f` then any range tombstones wider than the span
will be truncated to `[a-f)`. If the export hits a limit e.g. at `c`
then any MVCC range tombstones in the returned SST are truncated to
`[a-c)`.

If `StopMidKey` is enabled, then it's possible for two subsequent
exports to contain overlapping MVCC range tombstones. For example, given
the range tombstone `[a-f)`@5`,` if we return a resume key at `c@3` then
the response will contain a truncated MVCC range tombstone `[a-c\0)`@5``
which covers the point keys at `c`, but resuming from `c@3` will contain
the MVCC range tombstone `[c-f)`@5`` which overlaps with the MVCC range
tombstone in the previous response for the interval `[c-c\0)`@5`.`
`AddSSTable` will allow this overlap during ingestion once it supports
MVCC range tombstones.

Resolves #71398.

Release note: None

83282: cli/demo,cdc: enable rangefeeds by default r=[knz,stevendanna] a=HonoreDB

Fixes #82719,
which made it annoying to run changefeeds in cockroach demo
as you need to enable rangefeeds twice, once at the system
level. Now cockroach demo enables rangefeeds at startup.

You can override this behavior for performance or to demo
the process of enabling rangefeeds with the flag
`--auto-enable-rangefeeds=false`.

Release note (cli change): cockroach demo now enables rangefeeds by default. You can restore the old behavior with --auto-enable-rangefeeds=false.

Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Aaron Zinger <[email protected]>

pkg/cli/cliflags/flags.go

@@ -1295,6 +1295,12 @@ and the system tenant using the \connect command.`,
 If set, disable enterprise features.`,
 	}
 
+	DemoEnableRangefeeds = FlagInfo{
+		Name: "auto-enable-rangefeeds",
+		Description: `
+If set to false, overrides the default demo behavior of enabling rangefeeds.`,
+	}
+
 	UseEmptyDatabase = FlagInfo{
 		Name:        "empty",
 		Description: `Deprecated in favor of --no-example-database`,

pkg/cli/context.go

@@ -611,6 +611,7 @@ func setDemoContextDefaults() {
 	demoCtx.HTTPPort, _ = strconv.Atoi(base.DefaultHTTPPort)
 	demoCtx.WorkloadMaxQPS = 25
 	demoCtx.Multitenant = true
+	demoCtx.DefaultEnableRangefeeds = true
 
 	demoCtx.disableEnterpriseFeatures = false
 }

pkg/cli/demo.go

@@ -252,6 +252,12 @@ func runDemo(cmd *cobra.Command, gen workload.Generator) (resErr error) {
 	}
 	sqlCtx.ShellCtx.DemoCluster = c
 
+	if demoCtx.DefaultEnableRangefeeds {
+		if err = c.SetClusterSetting(ctx, "kv.rangefeed.enabled", true); err != nil {
+			return clierrorplus.CheckAndMaybeShout(err)
+		}
+	}
+
 	if cliCtx.IsInteractive {
 		cliCtx.PrintfUnlessEmbedded(`#
 # Welcome to the CockroachDB demo database!

pkg/cli/democluster/api.go

@@ -51,6 +51,10 @@ type DemoCluster interface {
 
 	// SetupWorkload initializes the workload generator if defined.
 	SetupWorkload(ctx context.Context) error
+
+	// SetClusterSetting overrides a default cluster setting at system level
+	// and for all tenants.
+	SetClusterSetting(ctx context.Context, setting string, value interface{}) error
 }
 
 // EnableEnterprise is not implemented here in order to keep OSS/BSL builds successful.

pkg/cli/democluster/context.go

@@ -96,6 +96,10 @@ type Context struct {
 	// Multitenant is true if we're starting the demo cluster in
 	// multi-tenant mode.
 	Multitenant bool
+
+	// DefaultEnableRangefeeds is true if rangefeeds should start
+	// out enabled.
+	DefaultEnableRangefeeds bool
 }
 
 // IsInteractive returns true if the demo cluster configuration

pkg/cli/democluster/demo_cluster.go

@@ -1217,6 +1217,28 @@ func (c *transientCluster) maybeEnableMultiTenantMultiRegion(ctx context.Context
 	return nil
 }
 
+func (c *transientCluster) SetClusterSetting(
+	ctx context.Context, setting string, value interface{},
+) error {
+	storageURL, err := c.getNetworkURLForServer(ctx, 0, false /* includeAppName */, false /* isTenant */)
+	if err != nil {
+		return err
+	}
+	db, err := gosql.Open("postgres", storageURL.ToPQ().String())
+	if err != nil {
+		return err
+	}
+	defer db.Close()
+	_, err = db.Exec(fmt.Sprintf("SET CLUSTER SETTING %s = '%v'", setting, value))
+	if err != nil {
+		return err
+	}
+	if c.demoCtx.Multitenant {
+		_, err = db.Exec(fmt.Sprintf("ALTER TENANT ALL SET CLUSTER SETTING %s = '%v'", setting, value))
+	}
+	return err
+}
+
 func (c *transientCluster) SetupWorkload(ctx context.Context) error {
 	if err := c.maybeEnableMultiTenantMultiRegion(ctx); err != nil {
 		return err

pkg/cli/flags.go

@@ -758,6 +758,7 @@ func init() {
 				"For details, see: "+build.MakeIssueURL(53404))
 
 		cliflagcfg.BoolFlag(f, &demoCtx.disableEnterpriseFeatures, cliflags.DemoNoLicense)
+		cliflagcfg.BoolFlag(f, &demoCtx.DefaultEnableRangefeeds, cliflags.DemoEnableRangefeeds)
 
 		cliflagcfg.BoolFlag(f, &demoCtx.Multitenant, cliflags.DemoMultitenant)
 		// TODO(knz): Currently the multitenant UX for 'demo' is not

pkg/cli/interactive_tests/test_demo_changefeeds.tcl

@@ -0,0 +1,42 @@
+#! /usr/bin/env expect -f
+
+source [file join [file dirname $argv0] common.tcl]
+
+start_test "Demo core changefeed using format=csv"
+spawn $argv demo --format=csv
+
+# We should start in a populated database.
+eexpect "movr>"
+
+# initial_scan=only prevents the changefeed from hanging waiting for more changes. 
+send "CREATE CHANGEFEED FOR users WITH initial_scan='only';\r"
+
+# header for the results of a successful changefeed
+eexpect "table,key,value"
+
+# Statement execution time after the initial scan completes
+eexpect "Time:"
+
+eexpect "movr>"
+send_eof
+eexpect eof
+
+end_test
+
+start_test "Demo with rangefeeds disabled as they are in real life"
+spawn $argv demo --format=csv --auto-enable-rangefeeds=false
+
+# We should start in a populated database.
+eexpect "movr>"
+
+# initial_scan=only prevents the changefeed from hanging waiting for more changes. 
+send "CREATE CHANGEFEED FOR users WITH initial_scan='only';\r"
+
+# changefeed should fail fast with an informative error.
+eexpect "ERROR: rangefeeds require the kv.rangefeed.enabled setting."
+
+eexpect "movr>"
+send_eof
+eexpect eof
+
+end_test

pkg/roachpb/api.proto

@@ -1529,6 +1529,15 @@ message ExportRequest {
   // large history being broken up into target_file_size chunks and prevent
   // blowing up on memory usage. This option is only allowed together with
   // return_sst since caller should reconstruct full tables.
+  //
+  // NB: If the result contains MVCC range tombstones, this can cause MVCC range
+  // tombstones in two subsequent SSTs to overlap. For example, given the range
+  // tombstone [a-f)@5, if we return a resume key at c@3 then the response will
+  // contain a truncated MVCC range tombstone [a-c\0)@5 which covers the point
+  // keys at c, but resuming from c@3 will contain the MVCC range tombstone
+  // [c-f)@5 which overlaps with the MVCC range tombstone in the previous
+  // response for the interval [c-c\0)@5. AddSSTable will allow this overlap
+  // during ingestion.
   bool split_mid_key = 13;
 
   // Return the exported SST data in the response.
@@ -1583,7 +1592,8 @@ message BulkOpSummary {
   reserved 4;
   // EntryCounts contains the number of keys processed for each tableID/indexID
   // pair, stored under the key (tableID << 32) | indexID. This EntryCount key
-  // generation logic is also available in the BulkOpSummaryID helper.
+  // generation logic is also available in the BulkOpSummaryID helper. It does
+  // not take MVCC range tombstones into account.
   map<uint64, int64> entry_counts = 5;
 }