sql: expose mvcc timestamps to SQL #51494

rohany · 2020-07-16T04:05:34Z

This PR adds introspection into the KV layer's concurrency control from
the SQL level. In particular, we now expose the MVCC HLC timestamp of a
row as a special system column on every table. This system column is
exposed as a decimal, and is computed as wall time * 10^10 + logical time.

To accomplish this, this PR adds planning and execution infrastructure
for implicit system columns that can be produced by the execution layer
for a particular row.

Release note (sql change): Expose the MVCC timestamp of each row as a
system column on tables. This column is named
crdb_internal_mvcc_timestamp and is accessible only in a limited set
of contexts.

cockroach-teamcity · 2020-07-16T04:05:41Z

This change is

rohany · 2020-07-16T14:34:09Z

pkg/sql/logictest/testdata/logic_test/mvcc

+statement error pq: INSERT has more expressions than target columns, 4 expressions for 3 targets
+INSERT INTO t VALUES (7, 8, 9, 1.0)
+
+# TODO (rohany): I'm not sure how to disable returning the system columns. The way


I'll need some pointers here.

rohany · 2020-07-16T14:35:36Z

pkg/sql/opt/exec/execbuilder/testdata/select_for_update

- └── scan   ·                   ·
-·           table               t@primary
-·           spans               FULL SCAN
+·     distribution      local


I'm not sure why these changed, my changes shouldn't have affected the plans like this. Though the plan does seem reasonable since the in is a tautology?

rohany · 2020-07-16T14:36:12Z

pkg/sql/opt/norm/prune_cols_funcs.go

+	// primary index. These columns are implicit and should not be part of the set
+	// of fetch columns, so we remove them here.
+	table := tabMeta.Table
+	for i := 0; i < table.ColumnCount(); i++ {


Because of the API change, there isn't an easy way to iterate over only the system columns of a table. I'm not sure that is a big deal though.

rohany · 2020-07-16T18:20:54Z

cc @RaduBerinde @andy-kimball

I've updated this to be on top of radu's changes. I've omitted the commit on top of this that adds the system column to the test catalog and updates the various opt tests. I went through all uses of ColumnCount and made a decision about whether to include the system column or not. I'll need some double checking about whether those decisions were correct + how to write tests that tickle those specific pieces of code.

RaduBerinde

Thanks! Some initial comments about cat

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rohany)

pkg/sql/opt/cat/column.go, line 32 at r2 (raw file):

	DeleteOnly
	// SystemColumn columns are implicit columns that every physical table
	// contains. These columns can only be read from and must not be included

Add that they cannot be part of the key cols / lax key cols for indexes.

pkg/sql/opt/cat/column.go, line 33 at r2 (raw file):

	// SystemColumn columns are implicit columns that every physical table
	// contains. These columns can only be read from and must not be included
	// as part of mutations. These columns are synthesized entirely within the

"synthesized entirely within the optimizer" is confusing. They are synthesized in the catalog implementation, not the optimizer (and their values are also synthesized outside the optimizer). I would just keep this detail out, it's not relevant to comprehending the interface.

pkg/sql/opt/cat/column.go, line 35 at r2 (raw file):

	// as part of mutations. These columns are synthesized entirely within the
	// optimizer and are not part of the persisted table definition.
	SystemColumn

[nit] the other enums don't have Column. I'd keep just as System even if it's kind of generic

pkg/sql/opt/cat/index.go, line 58 at r2 (raw file):

	// excludes any system columns. These columns shouldn't be considered by
	// statistics when computing the cost of scanning an index.
	ColumnCountNoSystemColumns() int

I'd remove this if possible. Looks like we are using it in just one place (in the coster). We could easily go through the columns and check how many are system columns. I think that code will one day have to look at each column's average value size (or something like that) anyway.

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

pkg/sql/opt/cat/column.go, line 32 at r2 (raw file):

Previously, RaduBerinde wrote…

Add that they cannot be part of the key cols / lax key cols for indexes.

Done

pkg/sql/opt/cat/column.go, line 33 at r2 (raw file):

Previously, RaduBerinde wrote…

"synthesized entirely within the optimizer" is confusing. They are synthesized in the catalog implementation, not the optimizer (and their values are also synthesized outside the optimizer). I would just keep this detail out, it's not relevant to comprehending the interface.

Done

pkg/sql/opt/cat/column.go, line 35 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] the other enums don't have Column. I'd keep just as System even if it's kind of generic

Done

pkg/sql/opt/cat/index.go, line 58 at r2 (raw file):

Previously, RaduBerinde wrote…

I'd remove this if possible. Looks like we are using it in just one place (in the coster). We could easily go through the columns and check how many are system columns. I think that code will one day have to look at each column's average value size (or something like that) anyway.

I added this because I didn't know if it was acceptable in the coster to iterate over all of the columns in the index. If that's fine i wont add this.

RaduBerinde

In cat.Table we should document how system columns interact with column families (I assume they don't show up in any family?)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rohany)

pkg/sql/logictest/testdata/logic_test/mvcc, line 111 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I'll need some pointers here.

I think in mutationBuilder.buildReturning, we'd want that inScope.appendColumnsFromTable call inside to skip system columns (just like we skip mutation columns)

pkg/sql/opt/cat/index.go, line 57 at r3 (raw file):

	// ColumnCount returns the number of columns in the index. This includes
	// columns that were part of the index definition (including the STORING
	// clause), as well as implicitly added primary key columns. This also the

remove "also the"

pkg/sql/opt/exec/execbuilder/testdata/select_for_update, line 528 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I'm not sure why these changed, my changes shouldn't have affected the plans like this. Though the plan does seem reasonable since the in is a tautology?

Not sure either.. I'll try to figure it out. In any case, maybe change to IN (SELECT b FROM t). Then it shouldn't be correct to drop the join

pkg/sql/opt/norm/prune_cols_funcs.go, line 191 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

Because of the API change, there isn't an easy way to iterate over only the system columns of a table. I'm not sure that is a big deal though.

I think this would be more clear if we just did this check above instead of cols.UnionWith(indexCols). I think that's the only way a system column would make its way into this set in the first place.

rohany

Done, added a blurb on the System columnkind

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

pkg/sql/logictest/testdata/logic_test/mvcc, line 111 at r1 (raw file):

Previously, RaduBerinde wrote…

I think in mutationBuilder.buildReturning, we'd want that inScope.appendColumnsFromTable call inside to skip system columns (just like we skip mutation columns)

That worked. I think that it is reasonable to skip system columns in appendColumnsFromTable so we don't accidentally add system columns.

pkg/sql/opt/cat/index.go, line 57 at r3 (raw file):

Previously, RaduBerinde wrote…

remove "also the"

done

pkg/sql/opt/exec/execbuilder/testdata/select_for_update, line 528 at r1 (raw file):

Previously, RaduBerinde wrote…

Not sure either.. I'll try to figure it out. In any case, maybe change to IN (SELECT b FROM t). Then it shouldn't be correct to drop the join

We produce joins with the select b from t.

pkg/sql/opt/norm/prune_cols_funcs.go, line 191 at r1 (raw file):

Previously, RaduBerinde wrote…

I think this would be more clear if we just did this check above instead of cols.UnionWith(indexCols). I think that's the only way a system column would make its way into this set in the first place.

Done

RaduBerinde

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

pkg/sql/logictest/testdata/logic_test/mvcc, line 111 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

That worked. I think that it is reasonable to skip system columns in appendColumnsFromTable so we don't accidentally add system columns.

Looks like that would be correct for all call sites, maybe rename to appendOrdinaryColumnsFromTable

RaduBerinde · 2020-07-17T17:38:51Z

pkg/sql/opt/exec/execbuilder/testdata/select_for_update, line 528 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

We produce joins with the select b from t.

I've been trying to figure this out. I can't find a rule that would do this transformation (though we'll probably add it in the future), so something fishy must be going on. I can't use optsteps reproduce inside opt tests without the testcatalog change. I've been trying to hack the planning code but it's hard. I can try again once you get the testcatalog working too.

RaduBerinde · 2020-07-17T18:10:04Z

pkg/sql/opt/exec/execbuilder/testdata/select_for_update, line 528 at r1 (raw file):

Previously, RaduBerinde wrote…

I've been trying to figure this out. I can't find a rule that would do this transformation (though we'll probably add it in the future), so something fishy must be going on. I can't use optsteps reproduce inside opt tests without the testcatalog change. I've been trying to hack the planning code but it's hard. I can try again once you get the testcatalog working too.

Ah, I think I figured it out. We have a rule EliminateJoinUnderProjectLeft that would do this if there is a Project on top. With your change, we have a Project to eliminate the system column. I was able to get the same plan on master with SELECT a instead of SELECT *. So it's all good.

rohany · 2020-07-17T19:26:52Z

Ah, I think I figured it out. We have a rule EliminateJoinUnderProjectLeft that would do this if there is a Project on top. With your change, we have a Project to eliminate the system column. I was able to get the same plan on master with SELECT a instead of SELECT *. So it's all good.

Nice. Why does that rule need a projection to fire though?

RaduBerinde · 2020-07-17T19:54:13Z

Nice. Why does that rule need a projection to fire though?

We are just missing a corresponding (simpler) rule when there is no projection. The rule with the projection does more things than eliminate an unnecessary semi-join.

rohany · 2020-07-17T19:55:23Z

that makes sense. @RaduBerinde do you think it's reasonable for me to start updating the opt tests, or there are more things here that you want to change?

RaduBerinde · 2020-07-17T21:14:20Z

Let me look over it more, I didn't get a chance to look at all the changes.

RaduBerinde

opt part LGTM. Unfortunately you will have to merge with Andy's changes in 51574.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rohany)

pkg/sql/opt/cat/utils.go, line 287 at r4 (raw file):

}

func formatColumn(col Column, isMutationCol bool, isSystemCol bool, buf *bytes.Buffer) {

[nit] should just pass the cat.ColumnKind

rohany · 2020-07-20T15:30:13Z

I rebased on top of andy's change by adding a system bool to the scopeColumn and not adding those columns to the fetchColIDs list. Let me know if that doesn't sound ideal.

I'll begin editing the opt tests as CI comes in.

RaduBerinde

LGTM, just some minor nits

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rohany)

pkg/sql/opt/optbuilder/fk_cascade.go, line 116 at r5 (raw file):

	// Set list of columns that will be fetched by the input expression.
	for i := range mb.outScope.cols {
		// Ensure that we don't add system columns to the fetch ords.

[nit] "fetch ords" is no longer a thing, maybe "fetch columns" (+2 more times in this file)

pkg/sql/opt/optbuilder/insert.go, line 521 at r5 (raw file):

	for i, n := 0, mb.tab.ColumnCount(); i < n && numCols < maxCols; i++ {
		// Skip mutation, hidden or system columns.
		if cat.IsMutationColumn(mb.tab, i) || mb.tab.Column(i).IsHidden() || cat.IsSystemColumn(mb.tab, i) {

maybe it's cleaner to check that the kind is Ordinary (same below)

pkg/sql/opt/optbuilder/mutation_builder.go, line 244 at r5 (raw file):

	// Set list of columns that will be fetched by the input expression.
	for i := range mb.outScope.cols {
		// Ensure that we don't add system columns to the fetch ords.

"fetch columns"

pkg/sql/opt/optbuilder/mutation_builder.go, line 371 at r5 (raw file):

	// Set list of columns that will be fetched by the input expression.
	for i := range mb.outScope.cols {
		// Ensure that we don't add system columns to the fetch ords.

"fetch columns"

pkg/sql/opt/optbuilder/mutation_builder.go, line 836 at r5 (raw file):

		private.ReturnCols = make(opt.ColList, mb.tab.ColumnCount())
		for i, n := 0, mb.tab.ColumnCount(); i < n; i++ {
			if cat.IsMutationColumn(mb.tab, i) || cat.IsSystemColumn(mb.tab, i) {

[nit] kind == Ordinary

pkg/sql/opt/optbuilder/scope.go, line 230 at r5 (raw file):

	}
	for i, n := 0, tab.ColumnCount(); i < n; i++ {
		if cat.IsMutationColumn(tab, i) || cat.IsSystemColumn(tab, i) {

Kind == Ordinary (I'd definitely do this here where the function name specifically mentions Ordinary)

pkg/sql/opt/optbuilder/scope_column.go, line 48 at r5 (raw file):

	// system is true if the column is an implicit system column. It should not
	// be included in mutations.
	system bool

[nit] We could also store the ColumnKind here instead. Not sure if that is better or worse though, your call

rohany · 2020-07-20T16:53:51Z

[nit] "fetch ords" is no longer a thing, maybe "fetch columns" (+2 more times in this file)

Done woops.

Updated all of these checks to check != cat.Ordinary

[nit] We could also store the ColumnKind here instead. Not sure if that is better or worse though, your call

I think this one is easier to keep as the booleans.

Fixes cockroachdb#50102. This PR adds introspection into the KV layer's concurrency control from the SQL level. In particular, we now expose the MVCC HLC timestamp of a row as a special system column on every table. This system column is exposed as a decimal, and is computed as `wall time * 10^10 + logical time`. To accomplish this, this PR adds planning and execution infrastructure for implicit system columns that can be produced by the execution layer for a particular row. Release note (sql change): Expose the MVCC timestamp of each row as a system column on tables. This column is named `crdb_internal_mvcc_timestamp` and is accessible only in a limited set of contexts.

This commit adds the MVCC timestamp system column to the test catalog and updates all of the tests with the change. Release note: None

rohany · 2020-07-21T16:19:49Z

Alright, I've got the LGTM from @RaduBerinde. Going to merge this now!

bors r=raduberinde

craig · 2020-07-21T16:54:48Z

Build succeeded

GitHub CI (Cockroach)

rohany force-pushed the mvcc-col-again branch 2 times, most recently from 2750048 to 842cb0f Compare July 16, 2020 14:33

rohany commented Jul 16, 2020

View reviewed changes

rohany force-pushed the mvcc-col-again branch 2 times, most recently from 45f39c2 to d77df5d Compare July 16, 2020 17:25

RaduBerinde reviewed Jul 16, 2020

View reviewed changes

rohany force-pushed the mvcc-col-again branch from d77df5d to 8fd30ac Compare July 16, 2020 20:01

rohany commented Jul 16, 2020

View reviewed changes

RaduBerinde reviewed Jul 17, 2020

View reviewed changes

rohany force-pushed the mvcc-col-again branch 2 times, most recently from d17d080 to 136ba12 Compare July 17, 2020 03:46

rohany commented Jul 17, 2020

View reviewed changes

RaduBerinde reviewed Jul 17, 2020

View reviewed changes

rohany force-pushed the mvcc-col-again branch from 136ba12 to cecb2fe Compare July 17, 2020 16:41

RaduBerinde reviewed Jul 20, 2020

View reviewed changes

rohany force-pushed the mvcc-col-again branch from cecb2fe to 55193b5 Compare July 20, 2020 15:22

rohany force-pushed the mvcc-col-again branch from 3b90478 to 7b317f8 Compare July 20, 2020 16:00

RaduBerinde reviewed Jul 20, 2020

View reviewed changes

rohany force-pushed the mvcc-col-again branch from 7b317f8 to 25a651e Compare July 20, 2020 16:51

rohany force-pushed the mvcc-col-again branch from 25a651e to 9a8362b Compare July 21, 2020 03:52

rohany marked this pull request as ready for review July 21, 2020 03:52

rohany requested a review from a team July 21, 2020 03:52

rohany requested a review from a team as a code owner July 21, 2020 03:52

rohany requested review from pbardea and removed request for a team July 21, 2020 03:52

rohany mentioned this pull request Jul 21, 2020

sql: expose mvcc timestamps to SQL #50519

Closed

rohany changed the title ~~WIP (different approach) sql: expose mvcc timestamps to SQL~~ sql: expose mvcc timestamps to SQL Jul 21, 2020

pbardea removed their request for review July 21, 2020 12:52

rohany force-pushed the mvcc-col-again branch from 9a8362b to a23c405 Compare July 21, 2020 14:30

rohany force-pushed the mvcc-col-again branch from a23c405 to 6cb99c3 Compare July 21, 2020 14:33

opt: update opt test catalog to include system column and update tests

8e74024

This commit adds the MVCC timestamp system column to the test catalog and updates all of the tests with the change. Release note: None

rohany force-pushed the mvcc-col-again branch from 6cb99c3 to 8e74024 Compare July 21, 2020 15:47

craig bot merged commit 59cca11 into cockroachdb:master Jul 21, 2020

ericharmeling mentioned this pull request Aug 13, 2020

Large deletes guidance and TTL workarounds cockroachdb/docs#7999

Merged

lnhsingh mentioned this pull request Aug 26, 2020

sql: expose mvcc timestamps to SQL cockroachdb/docs#8137

Closed

ajwerner mentioned this pull request Dec 17, 2020

kv,storage: omit MVCC timestamps for values read from intents of the current transaction #58046

Open

tnarg mentioned this pull request May 23, 2022

Add CockroachDB support sqlc-dev/sqlc#1389

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: expose mvcc timestamps to SQL #51494

sql: expose mvcc timestamps to SQL #51494

rohany commented Jul 16, 2020 •

edited

Loading

cockroach-teamcity commented Jul 16, 2020

rohany Jul 16, 2020

rohany Jul 16, 2020

rohany Jul 16, 2020

rohany commented Jul 16, 2020

RaduBerinde left a comment

rohany left a comment

RaduBerinde left a comment

rohany left a comment

RaduBerinde left a comment

RaduBerinde commented Jul 17, 2020

RaduBerinde commented Jul 17, 2020

rohany commented Jul 17, 2020

RaduBerinde commented Jul 17, 2020

rohany commented Jul 17, 2020

RaduBerinde commented Jul 17, 2020

RaduBerinde left a comment

rohany commented Jul 20, 2020

RaduBerinde left a comment

rohany commented Jul 20, 2020

rohany commented Jul 21, 2020

craig bot commented Jul 21, 2020

sql: expose mvcc timestamps to SQL #51494

sql: expose mvcc timestamps to SQL #51494

Conversation

rohany commented Jul 16, 2020 • edited Loading

cockroach-teamcity commented Jul 16, 2020

rohany Jul 16, 2020

Choose a reason for hiding this comment

rohany Jul 16, 2020

Choose a reason for hiding this comment

rohany Jul 16, 2020

Choose a reason for hiding this comment

rohany commented Jul 16, 2020

RaduBerinde left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

RaduBerinde commented Jul 17, 2020

RaduBerinde commented Jul 17, 2020

rohany commented Jul 17, 2020

RaduBerinde commented Jul 17, 2020

rohany commented Jul 17, 2020

RaduBerinde commented Jul 17, 2020

RaduBerinde left a comment

Choose a reason for hiding this comment

rohany commented Jul 20, 2020

RaduBerinde left a comment

Choose a reason for hiding this comment

rohany commented Jul 20, 2020

rohany commented Jul 21, 2020

craig bot commented Jul 21, 2020

Build succeeded

rohany commented Jul 16, 2020 •

edited

Loading