sql: introduce system.settings table

[dt]

this will be used to store and manage cluster-wide, runtime changable settings.
this introduces the new table via a migration -- later work will use it to provide
settings accessors for calling code.

[cockroach-teamcity]

This change is 

[benesch]

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

pkg/migrations/migrations.go, line 61 at r1 (raw file):

		workFn:         createSettingsTable,
		newDescriptors: 1,
		newRanges:      0, // settings doesn't count, since its gossip range.

nit: "it's"

(sorry)

---

pkg/migrations/migrations.go, line 352 at r1 (raw file):

		b.CPut(sqlbase.MakeNameMetadataKey(desc.GetParentID(), desc.GetName()), desc.GetID(), nil)
		b.CPut(sqlbase.MakeDescMetadataKey(desc.GetID()), sqlbase.WrapDescriptor(&desc), nil)
		txn.SetSystemConfigTrigger()

I think make check is about to complain about this unchecked error.

---

pkg/sql/sqlbase/system.go, line 121 at r1 (raw file):

CREATE TABLE system.settings (
	name              STRING    NOT NULL PRIMARY KEY,
	value            	STRING    NOT NULL,

This seems misaligned. Stray tab?

---

pkg/sql/sqlbase/system.go, line 125 at r1 (raw file):

	valueType         STRING,
	FAMILY (name, value, lastUpdated, valueType)
);`

This entire def should be in the previous const declaration for system config tables; this declaration is for non-config tables.

---

pkg/sql/sqlbase/system.go, line 177 at r1 (raw file):

	// the use of a validating, logging accessor, so we'll go ahead and tolerate
	// read-only privs to make that migration possible later.
	keys.SettingsTableID: {privilege.ReadWriteData, privilege.ReadData},

👍

I'd move this between Zones and Lease, though, to keep it with the other system config tables.

---

pkg/sql/sqlbase/system.go, line 521 at r1 (raw file):

		PrimaryIndex:   pk("name"),
		NextIndexID:    2,
		Privileges:     NewPrivilegeDescriptor(security.RootUser, SystemDesiredPrivileges(keys.SettingsTableID)),

This should also be up in the previous var block.

---

pkg/sql/testdata/logic_test/system, line 149 at r1 (raw file):

created  TIMESTAMP  false  now()           {jobs_status_created_idx}
payload  BYTES      false  NULL            {}

Can you add SHOW COLUMNS FROM system.settings here for consistency?

---

pkg/sql/testdata/logic_test/system, line 231 at r1 (raw file):

jobs  root  SELECT
jobs  root  UPDATE

and SHOW GRANTS ON system.settings here?

---

Comments from Reviewable

[dt]

Review status: 4 of 7 files reviewed at latest revision, 8 unresolved discussions.

---

pkg/migrations/migrations.go, line 61 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

nit: "it's"

(sorry)

Done.

---

pkg/migrations/migrations.go, line 352 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I think make check is about to complain about this unchecked error.

Done.

---

pkg/sql/sqlbase/system.go, line 121 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

This seems misaligned. Stray tab?

Done.

---

pkg/sql/sqlbase/system.go, line 125 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

This entire def should be in the previous const declaration for system config tables; this declaration is for non-config tables.

Done.

---

pkg/sql/sqlbase/system.go, line 177 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

👍

I'd move this between Zones and Lease, though, to keep it with the other system config tables.

Done.

---

pkg/sql/sqlbase/system.go, line 521 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

This should also be up in the previous var block.

Done.

---

pkg/sql/testdata/logic_test/system, line 149 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Can you add SHOW COLUMNS FROM system.settings here for consistency?

Done.

---

pkg/sql/testdata/logic_test/system, line 231 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

and SHOW GRANTS ON system.settings here?

Done.

---

Comments from Reviewable

[benesch]

Nice, that's all of the comments I have! (Though looks like TestZip is legitimately failing because its output depends on the installed system tables.)

Holding off on the LGTM because I don't know what protocol is for implementations that precede their RFC.

---

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[benesch]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[dt]

Fixed TestZip.

I'll update the RFC to in-progress and merge it.

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[danhhz]

lgtm, though sciencedog on the privilege stuff

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable