sql: Display inherited constraints in SHOW PARTITIONS

[rohany]

SHOW PARTITIONS now displays the inherited zone configuration of the
partitions in a separate column. To accomplish this, the
crdb_internal.zones table now holds on to the inherited constraints of
each zone in a separate column. Additionally, the
crdb_internal.partitions table holds on to the zone_id and subzone_id of
the zone configuration the partition refers to. These id's correspond to
the zone configuration at the lowest point in that partitions
"inheritance chain".

Release justification: Adds a low risk, good to have UX feature.

Fixes #40349.

Release note (sql change):

• SHOW PARTITIONS now displays inherited zone configurations.
• Adds the zone_id, subzone_id columns to crdb_internal.partitions,
  which form a link to the corresponding zone config in
  crdb_internal.zones which apply to the partitions.
• Rename the config_yaml, config_sql and config_proto columns in
  crdb_internal.zones to raw_config_yaml, raw_config_sql,
  raw_config_proto.
• Add the columns full_config_sql and full_config_yaml to the
  crdb_internal.zones table which display the full/inherited zone
  configuration.

[cockroach-teamcity]

This change is 

[rohany]

There are some problems with this that I'm noticing however, such as follows. Consider the example @andreimatei brought up in #40377 with the following table and zone constraints applied

create table t(country string, x int, primary key (country, x)) 
partition by list (country) 
(
  partition us values in ('us') partition by range (x) 
    (
      partition low values from (0) to (100)
    )
);

alter table t configure zone using constraints='[+region=us-east1]';
alter partition us of table t configure zone using constraints='[+az=b]';

If i understand zone inheritance correctly, partition low should have constraints +region=us-east1, because that is the first constraint above it in the inheritance chain of partition -> index -> table -> database -> default. That is what the output of show partitions now will display :

root@127.0.0.1:62780/movr> show partitions from table t;
  database_name | table_name | partition_name | parent_partition | column_names | index_name | partition_value |       zone_config       |               full_zone_config
+---------------+------------+----------------+------------------+--------------+------------+-----------------+-------------------------+-----------------------------------------------+
  movr          | t          | us             | NULL             | country      | t@primary  | ('us')          | constraints = '[+az=b]' | range_min_bytes = 16777216 (INHERITED),
                |            |                |                  |              |            |                 |                         | range_max_bytes = 67108864 (INHERITED),
                |            |                |                  |              |            |                 |                         | gc.ttlseconds = 90000 (INHERITED),
                |            |                |                  |              |            |                 |                         | num_replicas = 1 (INHERITED),
                |            |                |                  |              |            |                 |                         | constraints = [+az=b],
                |            |                |                  |              |            |                 |                         | lease_preferences = [] (INHERITED)
  movr          | t          | low            | us               | x            | t@primary  | (0) TO (100)    | NULL                    | range_min_bytes = 16777216 (INHERITED),
                |            |                |                  |              |            |                 |                         | range_max_bytes = 67108864 (INHERITED),
                |            |                |                  |              |            |                 |                         | gc.ttlseconds = 90000 (INHERITED),
                |            |                |                  |              |            |                 |                         | num_replicas = 1 (INHERITED),
                |            |                |                  |              |            |                 |                         | constraints = [+region=us-east1] (INHERITED),
                |            |                |                  |              |            |                 |                         | lease_preferences = [] (INHERITED)
(2 rows)

However, from what I can gather about zone config key resolution the zone config for keys in low will start with something like /Table/Index/1/us/ and thus resolve to have constraint +az=b. This makes sense, but I don't think follows the rules that we currently have laid out for zone configuration inheritance.

Please let me know if I am on the right track with this stuff, and loop in people who know more about this.

[rohany]

This is ready for a look now, I redid the implementation in a much better way. However, I still can't reproduce the example that andrei has created -- the configuration of the subpartition never "switches". Instead it always inherits from the table/indexes configuration, not the parent partitions.

[andreimatei]

Let me propose something else. Instead of adding the zone_config column to crdb_internal.partitions, add a reference to the crdb_internal.zones table (<zone_id/subzone_id>). subzone_id is brand new.
crdb_internal.zones doesn't deal with inheritance, but I think we should expand it to do it - besides its config_yaml column, add also an config_full_yaml one that inherits.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @awoods187, @rohany, and @solongordon)

---

pkg/sql/crdb_internal.go, line 2513 at r1 (raw file):

}

func addPartitioningRows(

pls comment this function and its arguments. What can be nil? The implementation is recursive, so this is quite important

---

pkg/sql/crdb_internal.go, line 2513 at r1 (raw file):

}

func addPartitioningRows(

nit: if this function has been introduced recently, I think a better name would be visitPartitions. This function does not add anything to anything; it calls a callback.
Also, I'd extract the conversion to different types of datums into that callback

---

pkg/sql/crdb_internal.go, line 2537 at r1 (raw file):

	colNames := tree.NewDString(buf.String())

	var a sqlbase.DatumAlloc

pls a better name than a :)

---

pkg/sql/crdb_internal.go, line 2546 at r1 (raw file):

	}

	for _, l := range partitioning.List {

please comment around here that what this does is produce the partition_value column

---

pkg/sql/crdb_internal.go, line 2561 at r1 (raw file):

		}
		name := tree.NewDString(l.Name)
		_, zone, subzone, err := GetZoneConfigInTxn(ctx, p.txn, uint32(table.ID), index, l.Name, false)

pls comment the false inline
https://github.com/cockroachdb/cockroach/blob/master/docs/style.md#inline-comments-for-arguments-to-function-calls

Or do everybody one better and, in a different commit, change GetZoneConfigInTxn() to take an enum instead of a naked bool so that callers are naturally readable.

---

pkg/sql/crdb_internal.go, line 2572 at r1 (raw file):

			Partition: tree.Name(l.Name),
		}
		// TODO (rohany): is it correct to take zone if the subzone is nil?

I think so

---

pkg/sql/crdb_internal.go, line 2588 at r1 (raw file):

			numColumns,
			colNames,
			tree.NewDString(buf.String()),

pls comment (or assign it to a variable before)

---

pkg/sql/crdb_internal.go, line 2589 at r1 (raw file):

			colNames,
			tree.NewDString(buf.String()),
			tree.DNull,

pls comment

[rohany]

I thought about changing crdb_internal.zones instead, but this becomes a problem when a partition doesn't have a zone config, because then it doesn't show up in crdb_internal.zones. This makes it not really clear what to do when trying to figure out the inheritance information relating to a partition. It seemed more straightforward to instead do this on crdb_internal.partitions because there we know all of the partitions and can then look into the zones table for information. Let's figure this out before fixing the code related comments.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @rohany, and @solongordon)

[andreimatei]

This makes it not really clear what to do

It seems clear enough to me: we link the zoneless partition to the lowest ancestor that does have a zone. And that's the end of it when talking about the partitions table.
Then, in the context of the zones table, we deal with inheritance by exposing both the raw zone config for each zone (as it's implemented now) and the "completed" one (after applying inheritance) in a new column.

[andreimatei]

Btw, longer term, I'm convinced that the model generally needs to evolve away from a partition "having" a zone to a partition "being linked to a zone" (i.e. away from a 1-1 relationship between zones and partitions and to a many-one relationship). So the linking that I'm suggesting we do now I think will naturally evolve into that.

[rohany]

That makes sense, but I still don't think it's that easy to implement that. At least right now, the show commands are just translated into joins (in this case joining partitions and zones). At this point when we see a partition without a zone configuration, we can't just follow the chain of inheritance up, because we are at the sql level, not go anymore. We could change the show partitions implementation to instead be a plan node and get executed, but I think that would be too large of a change (I haven't done something like that before so I'm unsure of the scope of that).

[andreimatei]

I'm proposing we "walk the chain" in the generator for the crdb_internal.partitions table, not in the implementation of show partitions. Btw, there's code that I just wrote for that; maybe it helps:
https://github.com/andreimatei/cockroach/blob/91bb5880bc26a1447ed8a2ed6d6ddd900a863048/pkg/storage/reports/reporter.go#L289

[rohany]

Ok, I think I understand your proposed solution now, sorry it took so long. To reiterate:

1. Add a link from crdb_internal.partitions to crdb_internal.zones which represents the zone config that this partition is applying to. Here we pick the correct zone config based on the inheritance chain. There should be an easy way to generate a span when given a partition right?
2. The crdb_internal.zones table should display the inherited zone constraints for each zone in a separate column.
3. Show partitions should just grab the appropriate zone info columns (applied + inherited) for the id's that they link to.

I like this solution, and it should be more useful going forward than what I have. Thanks for the feedback andrei!

[rohany]

PTAL, i reimplemented it in the different way, and I think the code is better now.

[andreimatei]

pls put more info in the release note to help the docs folk understand what needs to be documented. Speak about the new columns.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @awoods187, @rohany, and @solongordon)

---

pkg/ccl/logictestccl/testdata/logic_test/partitioning, line 863 at r2 (raw file):

RESET sql_safe_updates

<<<<<<< HEAD

hide your porn

---

pkg/sql/crdb_internal.go, line 2106 at r2 (raw file):

                           -- possible (e.g. the object was deleted).
  config_protobuf  BYTES NOT NULL,
	inherited_config_yaml STRING NOT NULL,

alignment

---

pkg/sql/crdb_internal.go, line 2106 at r2 (raw file):

                           -- possible (e.g. the object was deleted).
  config_protobuf  BYTES NOT NULL,
	inherited_config_yaml STRING NOT NULL,

s/inherited/full

Better yet, I think we should rename the existing columns raw_config and call the new ones config. This is technically a breaking change, but people are gonna start getting what they were expecting in the column named config.
Also, I think we should get rid of the [inherited_]config_sql columns and turn zoneConfigToSQL() into a builtin function that takes the yaml.

Let's get more eyes on these schema changes - cc @bdarnell @knz

---

pkg/sql/crdb_internal.go, line 2161 at r2 (raw file):

			// Inherit full information about this zone.
			fullZone := configProto
			// if fullZone is a SubzonePlaceholder, remove its SubzonePlaceholder marker so that

nit:s/if/If

---

pkg/sql/crdb_internal.go, line 2164 at r2 (raw file):

			// we get the correct inheritance information.
			if fullZone.IsSubzonePlaceholder() {
				fullZone.NumReplicas = nil

How come we have to do this crap? Can we improve completeZoneConfig so that we don't have to do it?

---

pkg/sql/crdb_internal.go, line 2170 at r2 (raw file):

			}

			if !configProto.IsSubzonePlaceholder() {

I say we get rid of this "subzone placeholder" concept (in general, but in particular for the purposes of this table). I don't understand why this concept exists, in general; there's nothing special about a "subzone placeholder" zone as opposed to any other zone. I think it had to exist in the dark times before cascading zone configs, but now classifying zones as "normal" and "placeholders" I believe is a useless distinction. I say let's just add rows to the crdb_internal.zones table for these placeholders.

---

pkg/sql/crdb_internal.go, line 2644 at r2 (raw file):

		// Figure out which zone and subzone this partition should correspond to.
		zoneID, zone, subzone, err := GetZoneConfigInTxn(ctx, p.txn, uint32(table.ID), index, l.Name, false /* getInheritedDefault */)

long line

---

pkg/sql/crdb_internal.go, line 2648 at r2 (raw file):

			return err
		}
		subzoneID := 0

there's now a SubzoneID type in the storage/reports package, and a utility function to deal with the +1 index to id conversion. Move those to base and use them.

---

pkg/sql/crdb_internal.go, line 2700 at r2 (raw file):

		// Figure out which zone and subzone this partition should correspond to.
		zoneID, zone, subzone, err := GetZoneConfigInTxn(ctx, p.txn, uint32(table.ID), index, r.Name, false /* getInheritedDefault */)

long line

---

pkg/sql/crdb_internal.go, line 2749 at r2 (raw file):

	list_value  STRING,
	range_value STRING,
	zone_id INT,

please comment what these column reference - the crdb_internal.zones table
Better yet, have you checked if it's feasible to add a FK spec to this virtual table descriptor? Don't know if it's doable; you'll probably have to add an index to the zones' descriptor. But I think it's worth a shot to level up these tables.

[bdarnell]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @awoods187, @knz, @rohany, and @solongordon)

---

pkg/sql/crdb_internal.go, line 2106 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

s/inherited/full

Better yet, I think we should rename the existing columns raw_config and call the new ones config. This is technically a breaking change, but people are gonna start getting what they were expecting in the column named config.
Also, I think we should get rid of the [inherited_]config_sql columns and turn zoneConfigToSQL() into a builtin function that takes the yaml.

Let's get more eyes on these schema changes - cc @bdarnell @knz

I'm not close to this area, but I think this table should be a more-or-less direct reflection of what's in the ALTER... CONFIGURE ZONE statements: only the raw local configs. Merging in the inherited configs should be the job of higher-level functionality like SHOW ZONE CONFIGURATION (which I think should show the final configuration after all inheritance and other processing).

I thing I agree in principle with a conversion function instead of having both sql and yaml columns, but I'm not sure I'd remove them now that they're already there.

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @awoods187, @knz, @rohany, and @solongordon)

---

pkg/sql/crdb_internal.go, line 2106 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I'm not close to this area, but I think this table should be a more-or-less direct reflection of what's in the ALTER... CONFIGURE ZONE statements: only the raw local configs. Merging in the inherited configs should be the job of higher-level functionality like SHOW ZONE CONFIGURATION (which I think should show the final configuration after all inheritance and other processing).

I thing I agree in principle with a conversion function instead of having both sql and yaml columns, but I'm not sure I'd remove them now that they're already there.

Ben, keep in mind that crdb_internal.zones is already a "higher-level" thing than system.zones, which gives you raw data. That one is a too raw (it gives you protos, and it doesn't expand subzones). But we'll hopefully change it.
Does this change your mind?

[bdarnell]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @knz, @rohany, and @solongordon)

---

pkg/sql/crdb_internal.go, line 2106 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Ben, keep in mind that crdb_internal.zones is already a "higher-level" thing than system.zones, which gives you raw data. That one is a too raw (it gives you protos, and it doesn't expand subzones). But we'll hopefully change it.
Does this change your mind?

Maybe? I don't have strong opinions here because I don't know how this table is used.

[rohany]

friendly ping

[rohany]

@andreimatei do you mind giving this another quick look before I merge? I had to make a couple more changes to get things to pass/avoid changing other tests too much.

[andreimatei]

LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @knz, @rohany, and @solongordon)

---

pkg/sql/crdb_internal.go, line 2167 at r4 (raw file):

			// Write down information about the zone in the table.
			// TODO (rohany): We would like to just display information about these

many tests depend on these zone rows not existing for placeholder zones? Is it more than logic tests? (cause logic test golden files can be rewritten by running with -rewrite

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @awoods187, @knz, @rohany, and @solongordon)

---

pkg/ccl/logictestccl/testdata/logic_test/partitioning, line 863 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

hide your porn

:(

---

pkg/sql/crdb_internal.go, line 2513 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

pls comment this function and its arguments. What can be nil? The implementation is recursive, so this is quite important

Done.

---

pkg/sql/crdb_internal.go, line 2537 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

pls a better name than a :)

Done.

---

pkg/sql/crdb_internal.go, line 2546 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

please comment around here that what this does is produce the partition_value column

Done.

---

pkg/sql/crdb_internal.go, line 2561 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

pls comment the false inline
https://github.com/cockroachdb/cockroach/blob/master/docs/style.md#inline-comments-for-arguments-to-function-calls

Or do everybody one better and, in a different commit, change GetZoneConfigInTxn() to take an enum instead of a naked bool so that callers are naturally readable.

Done.

---

pkg/sql/crdb_internal.go, line 2572 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think so

Done.

---

pkg/sql/crdb_internal.go, line 2588 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

pls comment (or assign it to a variable before)

Done.

---

pkg/sql/crdb_internal.go, line 2589 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

pls comment

Done.

---

pkg/sql/crdb_internal.go, line 2106 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

alignment

Done.

---

pkg/sql/crdb_internal.go, line 2106 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

s/inherited/full

Better yet, I think we should rename the existing columns raw_config and call the new ones config. This is technically a breaking change, but people are gonna start getting what they were expecting in the column named config.
Also, I think we should get rid of the [inherited_]config_sql columns and turn zoneConfigToSQL() into a builtin function that takes the yaml.

Let's get more eyes on these schema changes - cc @bdarnell @knz

I hesitate on moving zoneConfigToSQL into a builtin function, as then we won't have access to the internal representation of the tree.ZoneSpecifier to correctly format/choose the version of the alter * command, and instead would have to let users choose the correct version instead.

---

pkg/sql/crdb_internal.go, line 2164 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

How come we have to do this crap? Can we improve completeZoneConfig so that we don't have to do it?

Done.

---

pkg/sql/crdb_internal.go, line 2170 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I say we get rid of this "subzone placeholder" concept (in general, but in particular for the purposes of this table). I don't understand why this concept exists, in general; there's nothing special about a "subzone placeholder" zone as opposed to any other zone. I think it had to exist in the dark times before cascading zone configs, but now classifying zones as "normal" and "placeholders" I believe is a useless distinction. I say let's just add rows to the crdb_internal.zones table for these placeholders.

Done.

---

pkg/sql/crdb_internal.go, line 2644 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

long line

Done.

---

pkg/sql/crdb_internal.go, line 2648 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

there's now a SubzoneID type in the storage/reports package, and a utility function to deal with the +1 index to id conversion. Move those to base and use them.

Done.

---

pkg/sql/crdb_internal.go, line 2700 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

long line

Done.

---

pkg/sql/crdb_internal.go, line 2749 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

please comment what these column reference - the crdb_internal.zones table
Better yet, have you checked if it's feasible to add a FK spec to this virtual table descriptor? Don't know if it's doable; you'll probably have to add an index to the zones' descriptor. But I think it's worth a shot to level up these tables.

I don't think its possible to add FK's / indexes to the virtual tables. It's unclear what these concepts really mean on virtual tables as well.

---

pkg/sql/crdb_internal.go, line 2167 at r4 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

many tests depend on these zone rows not existing for placeholder zones? Is it more than logic tests? (cause logic test golden files can be rewritten by running with -rewrite

They are some unit tests that seem to be checking the binary encoding of elements in the system.zones table. I can't remember the exact tests, but I'd be happy to try and fix those in a separate PR.

[rohany]

bors r=andreimatei

[craig]

Build succeeded

• GitHub CI (Cockroach)