Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

execbuilder: plan TableReaderSpec directly #47474

Closed
asubiotto opened this issue Apr 14, 2020 · 0 comments · Fixed by #49682
Closed

execbuilder: plan TableReaderSpec directly #47474

asubiotto opened this issue Apr 14, 2020 · 0 comments · Fixed by #49682
Assignees
@yuzefovich
Labels
A-sql-execution Relating to SQL execution. A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@asubiotto
Copy link
Contributor

This is the first step towards #47473. At a high level, we should create a new implementation of an exec.Factory and implement ConstructScan to return a TableReaderSpec directly. This behavior should be off by default. We might want to break down this issue further or expand once we've done a bit of investigation.
@asubiotto asubiotto added A-sql-execution Relating to SQL execution. A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Apr 14, 2020
This was referenced May 28, 2020
Merged
Merged
craig bot pushed a commit that referenced this issue Jun 2, 2020
@tbg @yuzefovich
Merge #49693 #49724
3b2245f 
49693: server: use "read-only" Gossip for tenants r=asubiotto,nvanbenschoten a=tbg

We were previously using the Gossip instance of the TestServer against
which the tenant was initialized.

This commit trims the dependency further by initializing its own Gossip
instance which is never written to (i.e. `AddInfo` is never called) and
which does not accept incoming connections.

As a reminder, the remaining problematic uses of Gossip as of this
commit are:

- making a `nodeDialer` (for `DistSender`), tracked in:
  #47909
- access to the system config:
  - `(schemaChangeGCResumer).Resume`, tracked:
    #49691
  - `database.Cache`, tracked:
    #49692
- `(physicalplan).spanResolver` (for replica oracle).
  This is likely not a blocker as we can use a "dumber" oracle in this case;
  the oracle is used for distsql physical planning of which tenants will
  do none. Tracked in: #48432

cc @ajwerner 

Release note: None

49724: sql: clean up of scanNode and some other things r=yuzefovich a=yuzefovich

**sql: unify PlanningCtx constructors into one**

Release note: None

**sql: remove separate scanVisilibity struct**

This commit removes `sql.scanVisibility` in favor of protobuf-generated
`execinfrapb.ScanVisibility`. It also introduces prettier aliases for
the two values into `execinfra` package that are now used throughout the
code.

Release note: None

**sql: clean up of scan node and a few other things**

This commit does the following cleanups of `scanNode`:
1. refactors `scanNode.initCols` method to be standalone (it will
probably be reused later by distsql spec exec factory).
2. removes `numBackfillColumns`, `specifiedIndexReverse`, and
`isSecondaryIndex` fields since they are no longer used.
3. refactors the code to remove `valNeededForCols` field which was
always consecutive numbers in range `[0, len(n.cols)-1]`.
4. refactors `getIndexIdx` method to not depend on `scanNode`.

Additionally, this commit removes `planDependencies` business
from `planTop` since optimizer now handles CREATE VIEW and handles
the plan dependencies on its own (and CREATE VIEW was the single
user of that struct in the plan top).

Also, it removes (which seems like) unnecessary call to `planColumns`
when creating distinct spec and an unused parameter from
`createTableReaders` method.

Addresses: #47474.

Release note: None

Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Jun 2, 2020
@yuzefovich
Merge #48338 #49348
464e5c4 
48338: workload/schemachange: classify schema change errors r=spaskob a=spaskob

Previously this workload will not fail even when a schema change
operation returns an error. Now we extract the SQLSTATE code from
the error and if it is of class `09`, i.e. Triggered Action Exception
or `XX`, i.e. Internal Error we abort the workload and report the error.
Also previously we aborted the txn at the first operation error, now
we continue unless the error is one of the classes above.
We also add a roachtest `schemachange/random-load` that runs
the workload and fails if unclassified errors are encountered.

Fixes #23286.

Release note: none.

49348: sql: distsql spec creation in execbuilder plumbing r=yuzefovich a=yuzefovich

**sql: add stub exec.Factory for opt-driven distsql planning**

This commit adds a stub implementation of `exec.Factory` interface that
will be creating DistSQL processor specs directly from `opt.Expr`,
sidestepping intermediate `planNode` phase.

It also introduces a new private cluster setting
"sql.defaults.experimental_distsql_planning" as well as a session
variable "experimental_distsql_planning" which determine whether the new
factory is used, set to `off` by default (other options are `on` and
`always` - the latter is only for the session variable).

`Off` planning mode means using the old code path, `on` means attempting
to use the new code path but falling back to the old one if we encounter
an error, and `always` means using only the new code path and do not
fallback in case of an error. Currently the fallback doesn't occur with
`always` only for SELECT statements (so that we could run other
statements types, like SET), meaning that when`always` option is used,
if we encounter an unsupported node while planning a SELECT statement,
an error is returned, but if we encounter an unsupported node while
planning a statement other than SELECT, we still fallback to the old
code (in a sense `always` behaves exactly like `on` for all statement
types except for SELECTs).

Release note: None

**sql: add plumbing for phys plan creation directly in execbuilder**

This commit introduces `planMaybePhysical` utility struct that
represents a plan and uses either planNode ("logical") or DistSQL spec
("physical") representations. It will be removed once we are able to
create all processor specs in execbuilder directly. This struct has been
plumbed in all places that need to look at the plan, but in most of them
only the logical representation is supported. However, the main codepath
for executing a statement supports both, and if physical representation
is used, then we bypass distsql physical planner.

This commit also renames `checkSupportForNode` to
`checkSupportForPlanNode` and `createPlanForNode` to
`createPhysPlanForPlanNode` to make their purpose more clear.

Addresses: #47474.

Release note: None

**sql: operate on pointers to PhysicalPlan**

This commit changes `createPlanFor*` methods to operate on pointers to
`PhysicalPlan` instead of values. It also renames and cleans up some
explain-related utility methods.

Release note: None

Co-authored-by: Spas Bojanov <spas@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@yuzefovich yuzefovich mentioned this issue Jun 2, 2020
Merged
@craig craig bot closed this as completed in 5e0cecf Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@yuzefovich yuzefovich

Labels
A-sql-execution Relating to SQL execution. A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: implement ConstructScan in the new factory yuzefovich/cockroach
2 participants
@yuzefovich @asubiotto