Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
49728: execgen: add generic inliner r=jordanlewis a=jordanlewis

This commit adds a new directive to execgen:

// execgen:inline

This directive causes a function that's annotated with it to be inlined
into all of its callers via AST transformation. This kind of inlining is
smarter than before - it permits arguments with names different from the
names of the function's formal parameters, for example.

This commit also changes the functions in distinct_tmpl to use this
directive instead of the manual templating method.

I think this is the only easy function to change. The rest have, at the
simple level of difficulty, static template-time parameters, and at a
harder level of difficulty, type parameters. Improving these cases
holistically should come next.

Release note: None

49841: sql: disallow cross database type references r=otan a=rohany

Fixes #49809.

This PR disallows using types from other databases in tables. This makes
certain behavior (like `DROP TYPE`) more predictable in their effects,
as well as unblocking some work for supporting user defined types in
`cockroach dump`.

Release note (sql change): Referencing types across databases has been
disabled.

49919: sql: use nicer apd.Decimal.SetInt64 in the code base r=yuzefovich a=yuzefovich

This commit is automatic replacement of `apd.Decimal.SetFinite((.*), 0)`
into a nicer `apd.Decimal.SetInt64($1)` which are equivalent.

Release note: None

49958: jobs: rename Resume to Unpause r=spaskob a=spaskob

Resume verb is already used to for the action of starting a job
after adoption on a node. Here instead Resume is the opposite
action of pausing a job.

Release note: none.

49961: opt: fix error caused by recursive CTE with zero rows on left side r=rytaft a=rytaft

Prior to this commit, a recursive CTE in which the cardinality of
the left side of the `UNION ALL` expression was zero would cause an
error in the statistics code, "estimated row count must be non-zero".
This was happening because the cardinality of the recursive CTE binding
props was set to be non-zero, but the row count, which came from the
left side expression, was not updated accordingly. The stats code only
allows the row count to be zero if the cardinality is also zero.

This commit fixes the problem by setting the row count of the binding
props to 1 if the estimated row count of the left side is less than 1.

Fixes #49911

Release note (bug fix): Fixed an internal planning error that occured
for recursive CTEs (WITH RECURSIVE expressions) in which the left side
of the UNION ALL query used in the CTE definition produced zero rows.

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Spas Bojanov <spas@cockroachlabs.com>
Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
  • Loading branch information
6 people committed Jun 8, 2020
6 parents 769897a + 3ccbdbb + 9a4aae5 + 3763073 + a1028aa + 6ff32cb commit a966fd4
Show file tree
Hide file tree
Showing 58 changed files with 1,197 additions and 351 deletions.
18 changes: 18 additions & 0 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Gopkg.toml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ ignored = [
name = "github.com/docker/docker"
branch = "master"

[[constraint]]
name = "github.com/dave/dst"
branch = "master"

[[constraint]]
name = "github.com/maruel/panicparse"
revision = "f20d4c4d746f810c9110e21928d4135e1f2a3efa"
Expand Down
8 changes: 4 additions & 4 deletions pkg/ccl/importccl/import_processor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -659,8 +659,8 @@ func TestCSVImportCanBeResumed(t *testing.T) {
resumePos := js.prog.ResumePos[0]
t.Logf("Resume pos: %v\n", js.prog.ResumePos[0])

// Resume the job and wait for it to complete.
if err := registry.Resume(ctx, nil, jobID); err != nil {
// Unpause the job and wait for it to complete.
if err := registry.Unpause(ctx, nil, jobID); err != nil {
t.Fatal(err)
}
js = queryJobUntil(t, sqlDB.DB, jobID, func(js jobState) bool { return jobs.StatusSucceeded == js.status })
Expand Down Expand Up @@ -763,8 +763,8 @@ func TestCSVImportMarksFilesFullyProcessed(t *testing.T) {
// Send cancellation and unblock import.
proceedImport()

// Resume the job and wait for it to complete.
if err := registry.Resume(ctx, nil, jobID); err != nil {
// Unpause the job and wait for it to complete.
if err := registry.Unpause(ctx, nil, jobID); err != nil {
t.Fatal(err)
}
js = queryJobUntil(t, sqlDB.DB, jobID, func(js jobState) bool { return jobs.StatusSucceeded == js.status })
Expand Down
4 changes: 2 additions & 2 deletions pkg/jobs/jobs.go
Original file line number Diff line number Diff line change
Expand Up @@ -361,11 +361,11 @@ func (j *Job) paused(ctx context.Context, fn func(context.Context, *kv.Txn) erro
})
}

// resumed sets the status of the tracked job to running or reverting iff the
// unpaused sets the status of the tracked job to running or reverting iff the
// job is currently paused. It does not directly resume the job; rather, it
// expires the job's lease so that a Registry adoption loop detects it and
// resumes it.
func (j *Job) resumed(ctx context.Context) error {
func (j *Job) unpaused(ctx context.Context) error {
return j.Update(ctx, func(txn *kv.Txn, md JobMetadata, ju *JobUpdater) error {
if md.Status == StatusRunning || md.Status == StatusReverting {
// Already resumed - do nothing.
Expand Down
8 changes: 4 additions & 4 deletions pkg/jobs/jobs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1094,11 +1094,11 @@ func TestJobLifecycle(t *testing.T) {
if err := exp.verify(job.ID(), jobs.StatusPaused); err != nil {
t.Fatal(err)
}
if err := registry.Resume(ctx, nil, *job.ID()); err != nil {
if err := registry.Unpause(ctx, nil, *job.ID()); err != nil {
t.Fatal(err)
}
// Resume the job again to ensure that the resumption is idempotent.
if err := registry.Resume(ctx, nil, *job.ID()); err != nil {
if err := registry.Unpause(ctx, nil, *job.ID()); err != nil {
t.Fatal(err)
}
if err := exp.verify(job.ID(), jobs.StatusRunning); err != nil {
Expand Down Expand Up @@ -1172,7 +1172,7 @@ func TestJobLifecycle(t *testing.T) {
if err := registry.CancelRequested(ctx, nil, *job.ID()); err != nil {
t.Fatal(err)
}
if err := registry.Resume(ctx, nil, *job.ID()); !testutils.IsError(err, "cannot be resumed") {
if err := registry.Unpause(ctx, nil, *job.ID()); !testutils.IsError(err, "cannot be resumed") {
t.Errorf("got unexpected status '%v'", err)
}
}
Expand All @@ -1183,7 +1183,7 @@ func TestJobLifecycle(t *testing.T) {
t.Fatal(err)
}
expectedErr := fmt.Sprintf("job with status %s cannot be resumed", jobs.StatusSucceeded)
if err := registry.Resume(ctx, nil, *job.ID()); !testutils.IsError(err, expectedErr) {
if err := registry.Unpause(ctx, nil, *job.ID()); !testutils.IsError(err, expectedErr) {
t.Errorf("expected '%s', but got '%v'", expectedErr, err)
}
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/jobs/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -709,13 +709,14 @@ func (r *Registry) Failed(ctx context.Context, txn *kv.Txn, id int64, causingErr
return job.WithTxn(txn).failed(ctx, causingError, nil)
}

// Resume resumes the paused job with id using the specified txn (may be nil).
func (r *Registry) Resume(ctx context.Context, txn *kv.Txn, id int64) error {
// Unpause changes the paused job with id to running or reverting using the
// specified txn (may be nil).
func (r *Registry) Unpause(ctx context.Context, txn *kv.Txn, id int64) error {
job, _, err := r.getJobFn(ctx, txn, id)
if err != nil {
return err
}
return job.WithTxn(txn).resumed(ctx)
return job.WithTxn(txn).unpaused(ctx)
}

// Resumer is a resumable job, and is associated with a Job object. Jobs can be
Expand Down
149 changes: 149 additions & 0 deletions pkg/sql/add_column.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
// Copyright 2020 The Cockroach Authors.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package sql

import (
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/schemaexpr"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
"github.com/cockroachdb/errors"
)

// addColumnImpl performs the logic of adding a column within an ALTER TABLE.
func (p *planner) addColumnImpl(
params runParams,
n *alterTableNode,
tn *tree.TableName,
desc *sqlbase.MutableTableDescriptor,
t *tree.AlterTableAddColumn,
) error {
d := t.ColumnDef
version := params.ExecCfg().Settings.Version.ActiveVersionOrEmpty(params.ctx)
toType, err := tree.ResolveType(params.ctx, d.Type, params.p.semaCtx.GetTypeResolver())
if err != nil {
return err
}
if supported, err := isTypeSupportedInVersion(version, toType); err != nil {
return err
} else if !supported {
return pgerror.Newf(
pgcode.FeatureNotSupported,
"type %s is not supported until version upgrade is finalized",
toType.SQLString(),
)
}

newDef, seqDbDesc, seqName, seqOpts, err := params.p.processSerialInColumnDef(params.ctx, d, tn)
if err != nil {
return err
}
if seqName != nil {
if err := doCreateSequence(
params,
n.n.String(),
seqDbDesc,
n.tableDesc.GetParentSchemaID(),
seqName,
n.tableDesc.Temporary,
seqOpts,
tree.AsStringWithFQNames(n.n, params.Ann()),
); err != nil {
return err
}
}
d = newDef
incTelemetryForNewColumn(d)

col, idx, expr, err := sqlbase.MakeColumnDefDescs(params.ctx, d, &params.p.semaCtx, params.EvalContext())
if err != nil {
return err
}
// If the new column has a DEFAULT expression that uses a sequence, add references between
// its descriptor and this column descriptor.
if d.HasDefaultExpr() {
changedSeqDescs, err := maybeAddSequenceDependencies(
params.ctx, params.p, n.tableDesc, col, expr, nil,
)
if err != nil {
return err
}
for _, changedSeqDesc := range changedSeqDescs {
if err := params.p.writeSchemaChange(
params.ctx, changedSeqDesc, sqlbase.InvalidMutationID, tree.AsStringWithFQNames(n.n, params.Ann()),
); err != nil {
return err
}
}
}

// We're checking to see if a user is trying add a non-nullable column without a default to a
// non empty table by scanning the primary index span with a limit of 1 to see if any key exists.
if !col.Nullable && (col.DefaultExpr == nil && !col.IsComputed()) {
span := n.tableDesc.PrimaryIndexSpan(params.ExecCfg().Codec)
kvs, err := params.p.txn.Scan(params.ctx, span.Key, span.EndKey, 1)
if err != nil {
return err
}
if len(kvs) > 0 {
return sqlbase.NewNonNullViolationError(col.Name)
}
}
_, err = n.tableDesc.FindActiveColumnByName(string(d.Name))
if m := n.tableDesc.FindColumnMutationByName(d.Name); m != nil {
switch m.Direction {
case sqlbase.DescriptorMutation_ADD:
return pgerror.Newf(pgcode.DuplicateColumn,
"duplicate: column %q in the middle of being added, not yet public",
col.Name)
case sqlbase.DescriptorMutation_DROP:
return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"column %q being dropped, try again later", col.Name)
default:
if err != nil {
return errors.AssertionFailedf(
"mutation in state %s, direction %s, and no column descriptor",
errors.Safe(m.State), errors.Safe(m.Direction))
}
}
}
if err == nil {
if t.IfNotExists {
return nil
}
return sqlbase.NewColumnAlreadyExistsError(string(d.Name), n.tableDesc.Name)
}

n.tableDesc.AddColumnMutation(col, sqlbase.DescriptorMutation_ADD)
if idx != nil {
if err := n.tableDesc.AddIndexMutation(idx, sqlbase.DescriptorMutation_ADD); err != nil {
return err
}
}
if d.HasColumnFamily() {
err := n.tableDesc.AddColumnToFamilyMaybeCreate(
col.Name, string(d.Family.Name), d.Family.Create,
d.Family.IfNotExists)
if err != nil {
return err
}
}

if d.IsComputed() {
computedColValidator := schemaexpr.NewComputedColumnValidator(params.ctx, n.tableDesc, &params.p.semaCtx)
if err := computedColValidator.Validate(d); err != nil {
return err
}
}

return nil
}
Loading

0 comments on commit a966fd4

Please sign in to comment.