sql schema: more support for per-table and per-index settings (storage parameters)

[michae2]

Some settings are best scoped to tables or indexes rather than sessions or users/roles. Four current examples of settings that we would like to configure per-table:

1. row-level GC TTL settings
2. table statistics collection settings
3. disallow_full_table_scans
4. row-size guardrails

One could imagine these settings being included in CREATE TABLE and CREATE INDEX statements, modified with ALTER TABLE and ALTER INDEX, and viewed with SHOW CREATE TABLE.

As @mgartner pointed out in his comment linked above, Postgres already has syntax for storage parameters for tables and indexes which it uses for per-table autovacuum, among other settings. This could be a logical place to collect CockroachDB-specific table settings as well.

We already parse CREATE TABLE ... WITH and CREATE INDEX ... WITH syntax, but then discard most storage parameters and do not persist them (edit: except for a few, see @otan's comment below). We do not parse ALTER TABLE ... SET (...) nor show storage parameters in SHOW CREATE TABLE output. As @rafiss noted in #43299 we don't have real support for any of the Postgres storage parameters, much less CockroachDB-specific storage parameters.

So I think there are three work items here:

• persist storage parameters in descriptors
• add support for ALTER TABLE ... SET (x = y) and ALTER COLUMN ... SET (x = y)
• show storage parameters in SHOW CREATE TABLE output

Edit: this overlaps with zone configs. (See discussion below.)

Jira issue: CRDB-8801

[otan]

but then discard the storage parameters and do not persist them in any way.

i disagree sir! we only discard some of them ... :P

cockroach/pkg/sql/catalog/catformat/index.go

Lines 159 to 239 in 605b549

	func formatStorageConfigs(
	table catalog.TableDescriptor, index *descpb.IndexDescriptor, f *tree.FmtCtx,
	) error {
	if index.GeoConfig.S2Geometry != nil || index.GeoConfig.S2Geography != nil {
	var s2Config *geoindex.S2Config
	if index.GeoConfig.S2Geometry != nil {
	s2Config = index.GeoConfig.S2Geometry.S2Config
	}
	if index.GeoConfig.S2Geography != nil {
	s2Config = index.GeoConfig.S2Geography.S2Config
	}
	defaultS2Config := geoindex.DefaultS2Config()
	numCustomSettings := 0
	if *s2Config != *defaultS2Config {
	for _, check := range []struct {
	key string
	val int32
	defaultVal int32
	}{
	{`s2_max_level`, s2Config.MaxLevel, defaultS2Config.MaxLevel},
	{`s2_level_mod`, s2Config.LevelMod, defaultS2Config.LevelMod},
	{`s2_max_cells`, s2Config.MaxCells, defaultS2Config.MaxCells},
	} {
	if check.val != check.defaultVal {
	if numCustomSettings > 0 {
	f.WriteString(", ")
	} else {
	f.WriteString(" WITH (")
	}
	numCustomSettings++
	f.WriteString(check.key)
	f.WriteString("=")
	f.WriteString(strconv.Itoa(int(check.val)))
	}
	}
	}
	if index.GeoConfig.S2Geometry != nil {
	col, err := table.FindColumnWithID(index.InvertedColumnID())
	if err != nil {
	return errors.Wrapf(err, "expected column %q to exist in table", index.InvertedColumnName())
	}
	defaultConfig, err := geoindex.GeometryIndexConfigForSRID(col.GetType().GeoSRIDOrZero())
	if err != nil {
	return errors.Wrapf(err, "expected SRID definition for %d", col.GetType().GeoSRIDOrZero())
	}
	cfg := index.GeoConfig.S2Geometry
	for _, check := range []struct {
	key string
	val float64
	defaultVal float64
	}{
	{`geometry_min_x`, cfg.MinX, defaultConfig.S2Geometry.MinX},
	{`geometry_max_x`, cfg.MaxX, defaultConfig.S2Geometry.MaxX},
	{`geometry_min_y`, cfg.MinY, defaultConfig.S2Geometry.MinY},
	{`geometry_max_y`, cfg.MaxY, defaultConfig.S2Geometry.MaxY},
	} {
	if check.val != check.defaultVal {
	if numCustomSettings > 0 {
	f.WriteString(", ")
	} else {
	f.WriteString(" WITH (")
	}
	numCustomSettings++
	f.WriteString(check.key)
	f.WriteString("=")
	f.WriteString(strconv.FormatFloat(check.val, 'f', -1, 64))
	}
	}
	}
	if numCustomSettings > 0 {
	f.WriteString(")")
	}
	}
	return nil
	}

[michae2]

A valid point, thank you for setting me straight, @otan! 😄

To be a little more clear about "persist storage parameters in descriptors", I would like storage parameters to have a common home in descriptors rather than being sent off to IndexDescriptor.GeoConfig.S2Geometry or some other custom part of the descriptor, so that we don't have to write bespoke put / get functions each time we add a storage parameter.

Perhaps I'm wishing for a pony, but I think adding a storage parameter should just take a little magical boilerplate, and almost no plumbing, similar to adding a cluster setting.

[michae2]

@jordanlewis points out that there's some overlap with this and zone configs, at least when it comes to TTL settings.

[ajwerner]

@jordanlewis points out that there's some overlap with this and zone configs, at least when it comes to TTL settings.

Heavy +1. One can think of zone configs as filling this niche: they are apply to portions of a descriptor. There are vague plans to move them to be stored on the descriptor after we rework the way that they are communicated to KV.

[ajwerner]

I'm not sure what this issue is asking for. We can put some stuff in descriptors. We want to put things related to KV behavior in zone configs, for now. We will be free to change that once @arulajmani and @irfansharif's project comes in.

[ajwerner]

See #66348

[ajwerner]

See also #66396.

[michae2]

I'm not sure what this issue is asking for.

This issue is asking for an easy, generic way to add storage parameters, which resembles the way we add cluster settings.

[ajwerner]

This issue is asking for an easy, generic way to add storage parameters, which resembles the way we add cluster settings.

You can just add them to the index descriptor proto and then make sure the plumbing from tree works properly. I don't think we need a framework per se. These just feel like your run-of-the-mill properties for a table or index. For that stuff we use the protobufs inside descpb/structured.proto and the interfaces in catalog

[ajwerner]

@michae2 I guess what I'm really looking for is to better understand why we need a "generic" facility here. "easy" feels like it's in the eye of the beholder. For any of the listed features at the top, we could add them and there'd be a straightforward enough way to do it. For 2-4, I'd just add fields to the table descriptor proto and do the plumbing through tree, catalog, and opt/cat to make the feature happen. That feels pretty easy to me. Is that answer unsatisfying?

[michae2]

@michae2 I guess what I'm really looking for is to better understand why we need a "generic" facility here. "easy" feels like it's in the eye of the beholder. For any of the listed features at the top, we could add them and there'd be a straightforward enough way to do it. For 2-4, I'd just add fields to the table descriptor proto and do the plumbing through tree, catalog, and opt/cat to make the feature happen. That feels pretty easy to me. Is that answer unsatisfying?

When you put it that way, it does sound pretty easy, even to this lazy beholder. 😁 But those plumbing steps don't include

• support for ALTER TABLE ... SET (x = y) and ALTER COLUMN ... SET (x = y) syntax
• updating documentation

I really love that cluster settings are as easy as a call to settings.RegisterXSetting() and my lazy programmer self wants that uncoupled simplicity everywhere. But maybe I'm asking for this too soon. Maybe it will take plumbing a few per-table settings before it will pay off to bottle the pattern up into a framework.