colexec: allow unneeded index columns to be skipped in cfetcher by rohany · Pull Request #43024 · cockroachdb/cockroach

rohany · 2019-12-06T17:17:17Z

It seems like all indexed columns were being marked as needed in
the cfetcher initialization steps, whether they were actually
in neededCols or not. This meant that unneeded columns weren't
actually getting skipped.

Release note: None

cockroach-teamcity · 2019-12-06T17:17:24Z

This change is

rohany · 2019-12-06T17:18:33Z

I'm not sure if this is correct, so running the test suite to see if my hunch is right. I noticed this by wondering why #43002 doesn't fix #42994. In the normal row fetcher all indexed columns are decoded, but the cfetcher has the ability to skip keys.

rohany · 2019-12-07T16:46:20Z

This depends on #43002 after the recent push.

jordanlewis

This needs some tests that prove that what you say is being added is actually true. Like, make a query that doesn't read all index columns and have some unsupported types in there.

It looks like it's failing CI due to an unsupported type thing?

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

jordanlewis · 2019-12-17T00:57:21Z

pkg/sql/colexec/cfetcher.go

 	// One value per column that is part of the key; each value is a column
 	// index (into cols); -1 if we don't need the value for that column.
 	indexColOrdinals []int
+	// allIndexColOrdinals is the same as allIndexColOrdinals but


This sentence references the same field twice

jordanlewis · 2019-12-17T00:57:23Z

pkg/sql/colexec/cfetcher.go

 	indexColOrdinals []int
+	// allIndexColOrdinals is the same as allIndexColOrdinals but
+	// does not contain any -1's. It is meant to be used only in logging.
+	allIndexColOrdinals []int


It seems like we use it for more than just logging, no?

we use it to just fill in all of the index columns when printing k/vs to the trace, as opposed to the needed set when tracekv is off

rohany · 2019-12-17T01:40:05Z

This PR actually depends on #43002 to pass the tests -- there are tests in master already that test what you're asking for, but this PR removed the code that allowed those tests to pass in favor of combining with #43002 to address those issues. I'll let it merge and update the PR.

It seems like all indexed columns were being marked as needed in the cfetcher initialization steps, whether they were actually in neededCols or not. This meant that unneeded columns weren't actually getting skipped. Release note: None

rohany · 2019-12-17T03:53:04Z

RFAL

yuzefovich · 2019-12-17T04:06:24Z

I think this PR fixes #43153, right?

rohany · 2019-12-17T04:10:36Z

I think so? I don't know how to find the schema of the tables in the queries to try it out though...

yuzefovich · 2019-12-17T05:08:52Z

You need to go to Artifacts tab of the build that failed. In there, you can find sqlsmith.log that contains all the queries that were executed before hitting the error / panic. For example, https://teamcity.cockroachdb.com/viewLog.html?buildId=1643186&buildTypeId=Cockroach_Nightlies_WorkloadNightly&tab=artifacts#%2Fsqlsmith%2Fsetup%3Dempty%2Fsetting%3Ddefault. Also, looking at the logs of the nodes can be helpful to see the stack trace (e.g. https://teamcity.cockroachdb.com/repository/download/Cockroach_Nightlies_WorkloadNightly/1643186:id/sqlsmith/setup%3Dempty/setting%3Ddefault/1.logs/cockroach.log).

rohany · 2019-12-17T15:03:14Z

Yes i see that, but it doesn't show me the initial set of defined tables available to SQLsmith -- the logs just start with the queries it is running. @mjibson where should I look?

yuzefovich · 2019-12-17T15:32:56Z

The config is called "empty", so there are no initial tables in that case. If there were, they would also be included in the sqlsmith.log.

rohany · 2019-12-20T21:02:52Z

Alright, I've confirmed that #43395 is fixed by this commit.

jordanlewis

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

rohany · 2019-12-20T22:32:30Z

bors r+

43024: colexec: allow unneeded index columns to be skipped in cfetcher r=rohany a=rohany It seems like all indexed columns were being marked as needed in the cfetcher initialization steps, whether they were actually in neededCols or not. This meant that unneeded columns weren't actually getting skipped. Release note: None Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>

craig · 2019-12-20T23:05:41Z

Build succeeded

GitHub CI (Cockroach)

yuzefovich · 2019-12-26T23:02:36Z

We should backport this commit and unrevert the revert (#43072), right guys? cc @asubiotto

rohany · 2019-12-30T03:20:26Z

This has to be backported too #43002 (and first) I think

asubiotto · 2020-01-13T17:15:06Z

OK. So to summarize we need to backport #43002, #43024 and then unrevert #43072 in that order?

rohany · 2020-01-13T17:16:48Z

Yes, i think that is correct.

rohany requested review from a team and jordanlewis December 6, 2019 17:17

rohany force-pushed the fetcher-needs-too-many branch from 8d1805e to 0a8b5fc Compare December 6, 2019 18:14

rohany mentioned this pull request Dec 6, 2019

colexec: cfetcher errors on setup when indexed column contains unhandled type #42999

Merged

rohany force-pushed the fetcher-needs-too-many branch 2 times, most recently from ef2a0c1 to 469ccb0 Compare December 7, 2019 16:46

jordanlewis reviewed Dec 17, 2019

View reviewed changes

rohany force-pushed the fetcher-needs-too-many branch from 469ccb0 to a8bbae1 Compare December 17, 2019 03:52

rohany requested a review from jordanlewis December 17, 2019 03:53

jordanlewis approved these changes Dec 20, 2019

View reviewed changes

craig bot merged commit a8bbae1 into cockroachdb:master Dec 20, 2019

yuzefovich mentioned this pull request Dec 20, 2019

roachtest: sqlsmith/setup=rand-tables/setting=no-ddl failed #43395

Closed

rohany mentioned this pull request Dec 23, 2019

roachtest: sqlsmith/setup=empty/setting=default failed #43153

Closed

asubiotto added the backport-19.2.x label Jan 13, 2020

rohany mentioned this pull request Jan 13, 2020

release-19.2: colexec: allow unneeded index columns to be skipped in cfetcher #43939

Merged

Conversation

rohany commented Dec 6, 2019

Uh oh!

cockroach-teamcity commented Dec 6, 2019

Uh oh!

rohany commented Dec 6, 2019

Uh oh!

rohany commented Dec 7, 2019

Uh oh!

jordanlewis left a comment

Choose a reason for hiding this comment

Uh oh!

jordanlewis Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

jordanlewis Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

rohany Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

rohany commented Dec 17, 2019

Uh oh!

rohany commented Dec 17, 2019

Uh oh!

yuzefovich commented Dec 17, 2019

Uh oh!

rohany commented Dec 17, 2019

Uh oh!

yuzefovich commented Dec 17, 2019

Uh oh!

rohany commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuzefovich commented Dec 17, 2019

Uh oh!

rohany commented Dec 20, 2019

Uh oh!

jordanlewis left a comment

Choose a reason for hiding this comment

Uh oh!

rohany commented Dec 20, 2019

Uh oh!

craig bot commented Dec 20, 2019

Build succeeded

Uh oh!

yuzefovich commented Dec 26, 2019

Uh oh!

rohany commented Dec 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asubiotto commented Jan 13, 2020

Uh oh!

rohany commented Jan 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rohany commented Dec 17, 2019 •

edited

Loading

rohany commented Dec 30, 2019 •

edited

Loading