sql: optimize point lookups on column families #30744

solongordon · 2018-09-27T21:18:06Z

For tables with multiple column families, point lookups will now only
scan column families which contain the needed columns. Previously we
would scan the entire row. This optimization allows for faster lookups
and, perhaps more importantly, reduces contention between operations on
the same row but disjoint column families.

Fixes #18168

Release note: None

cockroach-teamcity · 2018-09-27T21:18:12Z

This change is

solongordon · 2018-09-27T21:18:47Z

I'm going to add unit tests and additional logic tests, but the implementation is ready for a look if you're curious.

jordanlewis

The implementation looks great - very clean. The logic test failures are kind of weird. Why would this one have ended up looking at more spans than before?

testdata/select:578: SELECT message FROM [SHOW KV TRACE FOR SESSION]
	 WHERE message LIKE 'fetched:%' OR message LIKE 'output row%'
	expected:
	    fetched: /dt/primary/'2015-08-25 04:45:45.53453+00:00'/b -> '2015-08-25'
	    fetched: /dt/primary/'2015-08-25 04:45:45.53453+00:00'/c -> '2h45m2s234ms'
	    output row: ['2015-08-25 04:45:45.53453+00:00' '2015-08-25' '2h45m2s234ms']
	    
	but found (query options: "") :
	    fetched: /dt/primary/'2015-08-25 04:45:45.53453+00:00' -> NULL
	    fetched: /dt/primary/'2015-08-25 04:45:45.53453+00:00'/b -> '2015-08-25'
	    fetched: /dt/primary/'2015-08-25 04:45:45.53453+00:00'/c -> '2h45m2s234ms'
	    output row: ['2015-08-25 04:45:45.53453+00:00' '2015-08-25' '2h45m2s234ms']

Reviewed 7 of 7 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/opt_index_selection.go, line 687 at r1 (raw file):

	// least one non-nullable column in the needed column families, we can
	// potentially omit the primary family, since the primary keys are encoded
	// in all families.

Cool idea!

solongordon

Huh, I actually removed that first line from the expected output because that's the result I was seeing locally (and it seemed reasonable). But looks like Teamcity is still seeing the old result. I'll check it out. Also will look into why zone config tests are unhappy.

Reviewable status: complete! 0 of 0 LGTMs obtained

solongordon · 2018-10-01T15:43:20Z

Looks like this breaks the delete fast path, since that only deletes the spans from the delete node's underlying scan node. I'm looking into what the proper fix for that should be but open to suggestions.

root@127.0.0.1:52406/defaultdb> create table t (x int primary key, y int, z int, family (y), family (z));
CREATE TABLE

root@127.0.0.1:52406/defaultdb> insert into t values (1, 2, 3);
INSERT 1

root@127.0.0.1:52406/defaultdb> delete from t where x = 1;
DELETE 1

root@127.0.0.1:52406/defaultdb> select * from t;
  x |  y   | z
+---+------+---+
  1 | NULL | 3
(1 row)

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/opt_index_selection.go, line 603 at r1 (raw file):

// non-adjacent column families should be scanned.
func spansFromConstraintSpan(
	tableDesc *sqlbase.TableDescriptor,

It's better if this function takes an input Spans and appends to it, or we end up allocating a temporary Spans for each logical spans

pkg/sql/opt_index_selection.go, line 687 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Cool idea!

Except for composite datums :)

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/opt_index_selection.go, line 603 at r1 (raw file):

Previously, RaduBerinde wrote…

It's better if this function takes an input Spans and appends to it, or we end up allocating a temporary Spans for each logical spans

Done

pkg/sql/opt_index_selection.go, line 687 at r1 (raw file):

Previously, RaduBerinde wrote…

Except for composite datums :)

Aha, good point. Adding that to the comment since I will surely forget about that exception.

solongordon

OK, I think I fixed the deletion issues. I don't love the solution, but it's the least hacky approach I could find. I'm disabling this optimization if the spans might be used for the delete fast path. The annoying part was that this requires the scanNode to be aware that it is the source for a delete. Best way I could find to do this was to set a flag during plan expansion. I'm certainly open to other suggestions.

I still intend to add more testing.

Reviewable status: complete! 0 of 0 LGTMs obtained

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/opt_index_selection.go, line 687 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Aha, good point. Adding that to the comment since I will surely forget about that exception.

Done.

jordanlewis

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/expand_plan.go, line 126 at r2 (raw file):

		// If the source of the delete is a scan node (optionally with a render on
		// top), mark it as such. Note that this parallels the logic in
		// canDeleteFast.

To minimize this hacky-ness, I would make sure to add a corresponding comment to canDeleteFast. If that implementation changes and this one doesn't, what happens?

Also, I'm not sure how feasible this is, but can you have the deleteNode declare all columns as required? Or does this mess other things up?

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/expand_plan.go, line 126 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

To minimize this hacky-ness, I would make sure to add a corresponding comment to canDeleteFast. If that implementation changes and this one doesn't, what happens?

Also, I'm not sure how feasible this is, but can you have the deleteNode declare all columns as required? Or does this mess other things up?

Yes, will do.

I fooled around with the required columns approach but didn't have much luck. From the perspective of the delete node it's already marking all columns as needed:

cockroach/pkg/sql/opt_needed.go

Lines 212 to 213 in 4e60d58

    
           case *deleteNode: 
        
           	setNeededColumns(n.source, allColumns(n.source))

but in many cases this is only a subset of the columns because there is a projection between it and the scan node. It's probably possible to make this work but it felt like I was heading down a hackier path than the scanNode flag approach.

jordanlewis

LGTM. Can you add a couple of EXPLAIN logic tests to ensure we don't regress?

pkg/sql/opt_index_selection.go

jordanlewis · 2018-10-09T15:16:58Z

pkg/sql/opt/exec/execbuilder/testdata/select_index

+  PRIMARY KEY (a, b),
+  FAMILY (a, b),
+  FAMILY (c),
+  FAMILY (d)


Nice tests. The main one I was thinking of that I don't see here is a test that two adjacent non-primary families get coalesced into a single span. I'm sure it works today, since similar logic triggers for the primary-adjacent families, but this seems like a case that might regress as code changes.

jordanlewis · 2018-10-09T15:17:00Z

pkg/sql/opt/exec/execbuilder/testdata/select_index

@@ -1336,3 +1336,105 @@ render           ·         ·           (w)     ·
      │          spans     /1-/10      ·       ·
      └── scan   ·         ·           (v, w)  ·
 ·                table     t3@primary  ·       ·
+
+# ------------------------------------------------------------------------------
+# These tests are for the point lookup optimization, which applies to SELECTs


Might as well throw a quick explanation of the optimization in here.

pkg/sql/opt/exec/execbuilder/testdata/select_index

pkg/sql/opt_index_selection.go

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/opt/exec/execbuilder/testdata/select_index, line 1341 at r4 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Might as well throw a quick explanation of the optimization in here.

Done.

pkg/sql/opt/exec/execbuilder/testdata/select_index, line 1353 at r4 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Nice tests. The main one I was thinking of that I don't see here is a test that two adjacent non-primary families get coalesced into a single span. I'm sure it works today, since similar logic triggers for the primary-adjacent families, but this seems like a case that might regress as code changes.

Good idea, done.

For tables with multiple column families, point lookups will now only scan column families which contain the needed columns. Previously we would scan the entire row. This optimization allows for faster lookups and, perhaps more importantly, reduces contention between operations on the same row but disjoint column families. Fixes cockroachdb#18168 Release note: None

jordanlewis

LGTM

solongordon · 2018-10-09T17:21:40Z

bors r+

30744: sql: optimize point lookups on column families r=solongordon a=solongordon For tables with multiple column families, point lookups will now only scan column families which contain the needed columns. Previously we would scan the entire row. This optimization allows for faster lookups and, perhaps more importantly, reduces contention between operations on the same row but disjoint column families. Fixes #18168 Release note: None Co-authored-by: Solon Gordon <solon@cockroachlabs.com>

craig · 2018-10-09T17:38:02Z

Build succeeded

GitHub CI (Cockroach)

knz · 2018-10-12T10:47:42Z

Not sure this may want a backport?

solongordon · 2018-10-15T14:30:22Z

@nvanbenschoten and I discussed a backport and decided it was too risky for this late in the stability period.

solongordon requested review from jordanlewis, nvanbenschoten and a team September 27, 2018 21:18

solongordon requested a review from a team as a code owner September 27, 2018 21:18

solongordon requested a review from a team September 27, 2018 21:18

jordanlewis reviewed Sep 28, 2018

View reviewed changes

solongordon commented Sep 28, 2018

View reviewed changes

RaduBerinde approved these changes Oct 2, 2018

View reviewed changes

solongordon commented Oct 2, 2018

View reviewed changes

solongordon force-pushed the column-family-opt branch from b0468a1 to 8f1f62c Compare October 2, 2018 15:48

solongordon commented Oct 2, 2018

View reviewed changes

jordanlewis reviewed Oct 2, 2018

View reviewed changes

solongordon commented Oct 2, 2018

View reviewed changes

solongordon force-pushed the column-family-opt branch 2 times, most recently from 9edaa87 to 176c139 Compare October 2, 2018 19:09

nvanbenschoten mentioned this pull request Oct 4, 2018

sql: only fetch specific columns needed to validate check constraints for UPDATES #30707

Merged

jordanlewis approved these changes Oct 7, 2018

View reviewed changes

jordanlewis reviewed Oct 8, 2018

View reviewed changes

pkg/sql/opt_index_selection.go Show resolved Hide resolved

solongordon force-pushed the column-family-opt branch from 176c139 to a8980f6 Compare October 9, 2018 14:21

solongordon requested a review from a team October 9, 2018 14:21

jordanlewis reviewed Oct 9, 2018

View reviewed changes

solongordon commented Oct 9, 2018

View reviewed changes

solongordon force-pushed the column-family-opt branch from a8980f6 to ae97046 Compare October 9, 2018 15:38

jordanlewis approved these changes Oct 9, 2018

View reviewed changes

craig bot merged commit ae97046 into cockroachdb:master Oct 9, 2018

solongordon deleted the column-family-opt branch October 10, 2018 12:00

solongordon mentioned this pull request Jun 18, 2019

sql: tighter spans in index/lookup/zigzags joins #38280

Closed

jordanlewis mentioned this pull request Sep 18, 2019

sql: DELETE FROM is broken for system.jobs table #40890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: optimize point lookups on column families #30744

sql: optimize point lookups on column families #30744

solongordon commented Sep 27, 2018

cockroach-teamcity commented Sep 27, 2018

solongordon commented Sep 27, 2018

jordanlewis left a comment

solongordon left a comment

solongordon commented Oct 1, 2018

RaduBerinde left a comment

solongordon left a comment

solongordon left a comment

solongordon left a comment

jordanlewis left a comment

solongordon left a comment

jordanlewis left a comment

jordanlewis Oct 9, 2018

jordanlewis Oct 9, 2018

solongordon left a comment

jordanlewis left a comment

solongordon commented Oct 9, 2018

craig bot commented Oct 9, 2018

knz commented Oct 12, 2018

solongordon commented Oct 15, 2018

	case *deleteNode:
	setNeededColumns(n.source, allColumns(n.source))

sql: optimize point lookups on column families #30744

sql: optimize point lookups on column families #30744

Conversation

solongordon commented Sep 27, 2018

cockroach-teamcity commented Sep 27, 2018

solongordon commented Sep 27, 2018

jordanlewis left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

solongordon commented Oct 1, 2018

RaduBerinde left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

jordanlewis Oct 9, 2018

Choose a reason for hiding this comment

jordanlewis Oct 9, 2018

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

solongordon commented Oct 9, 2018

craig bot commented Oct 9, 2018

Build succeeded

knz commented Oct 12, 2018

solongordon commented Oct 15, 2018