GitLab

32954: storage: add interfaces for copysets r=tbg a=mvijaykarthik

Copysets significantly reduce the probability of data loss
in the presence of multi node failures. The RFC for copysets
is presented in https://github.com/cockroachdb/cockroach/pull/32816.

This commit adds interfaces and protos for copysets.
The implementation added later will have two independent tracks:
1. Assignment and persistance of store_id-copyset_id mapping.
2. Rebalancer changes to rebalance replicas into copysets.

Release note: None
Co-authored-by: Vijay Karthik <[email protected]>

Copysets significantly reduce the probability of data loss
in the presence of multi node failures. The RFC for copysets
is presented in https://github.com/cockroachdb/cockroach/pull/32816.

This commit adds interfaces and protos for copysets.
The implementation added later will have two independent tracks:
1. Assignment and persistance of store_id-copyset_id mapping.
2. Rebalancer changes to rebalance replicas into copysets.

Release note: None

34531: ui: implement logical plan design r=celiala a=celiala

This commit implements the designs as specified in
Zeplin design "Logical Plans - v1":
https://app.zeplin.io/project/5c40e9dab2616abf79e3eb99/screen/5c4b6f23143de2bfcb12ae4e

Note that remaining design items will be addressed in future commits (tracked in #34545).

![logical-plans](https://user-images.githubusercontent.com/3051672/52312291-11070d80-2978-11e9-8ecc-42ab4a859371.gif)

Release note: None

34592: opt: Fix panic in SQLSmith query r=andy-kimball a=andy-kimball

A SQLSmith query exposed a bug in the expression interner. This bug caused
the interner to consider empty literal arrays to be equal, even if their
static types are different. For example:

  ARRAY[]:::string[]
  ARRAY[]:::int[]

An empty string array should not be treated as the same as an empty int
array. The fix is to consult the static type of the array in addition to the
types of its elements (but only when there are zero elements or one of the
elements is null). In addition, I took the opportunity to make the existing
Tuple interner code faster, by only comparing static types when lables or
nulls are present.

Fixes #34439

Release note: None
Co-authored-by: Celia La <[email protected]>
Co-authored-by: Andrew Kimball <[email protected]>

This commit implements the designs as specified in
Zeplin design "Logical Plans - v1":
https://app.zeplin.io/project/5c40e9dab2616abf79e3eb99/screen/5c4b6f23143de2bfcb12ae4e

Note that remaining design items will be addressed in future commits (tracked in #34545).

![logical-plans](https://user-images.githubusercontent.com/3051672/52312291-11070d80-2978-11e9-8ecc-42ab4a859371.gif)

Release note: None

34501: tree: improve formatting API r=RaduBerinde a=RaduBerinde

This change improves the FmtCtx API: before, the caller had to save
old flags (or custom formatter callback) and then restore them. The
new API does this automatically:

```
oldFlags := ctx.ChangeFlags(x)          ctx.WithFlags(x, func() {
...                              -->      ...
ctx.ChangeFlags(oldFlags)               })
```

The BenchmarkFormatRandomStatements results are unaffected.

Release note: None
Co-authored-by: Radu Berinde <[email protected]>

34598: sql: plumb through context argument to IndexedRows methods r=yuzefovich a=yuzefovich

Adds plumbing for passing through a context which might be needed
for some implementations of GetRow() method.

Needed for: #34196. 

Release note: None
Co-authored-by: Yahor Yuzefovich <[email protected]ckroachlabs.com>

33244: storage: have sequenced reads use the intent history r=nvanbenschoten a=ridwanmsharif

Reads can now use the intent history to serve reads from
values written at a lower sequence. Reads no longer read
all writes in a transaction but instead read all writes
in the transaction with a higher sequence than it. If a key
is written to multiple times within a transaction, the read
can now get the appropriate value using the intent history.

Release note: None
Co-authored-by: Ridwan Sharif <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>

Adds plumbing for passing through a context which might be needed
for some implementations of GetRow() method.

Release note: None

A SQLSmith query exposed a bug in the expression interner. This bug caused
the interner to consider empty literal arrays to be equal, even if their
static types are different. For example:

  ARRAY[]:::string[]
  ARRAY[]:::int[]

An empty string array should not be treated as the same as an empty int
array. The fix is to consult the static type of the array in addition to the
types of its elements (but only when there are zero elements or one of the
elements is null).

In addition, I took the opportunity to make the existing Tuple interner code
faster, by only comparing static types when lables or nulls are present. I
also fixed the bytes reuse code by storing back resized byte arrays.

Fixes #34439

Release note: None

34566: storage/roachpb: various cleanup around Refresh{Range}Request r=nvanbenschoten a=nvanbenschoten

This PR includes a few small changes that I noticed while investigating #34025.
Co-authored-by: Nathan VanBenschoten <[email protected]>

To do this, it needed to read at the previous sequence number before
writing at the current sequece number.

Release note: None

This also fixes one bug, where MVCCScan would get stuck reading the
same key and never advancing. The added test cases caught this.

Release note: None

We need a cluster version for this change. We make sure
to set this newly created `IgnoreSequence` to true each time we see a
cluster version is too low.

Release note: None

Reads can now use the intent history to serve reads from
values written at a lower sequence. Reads no longer read
all writes in a transaction but instead read all writes
in the transaction with a higher sequence than it. If a key
is written to multiple times within a transaction, the read
can now get the appropriate value using the intent history.

Release note: None

34365: sql: fix FK validation join implementation r=lucy-zhang a=lucy-zhang

This PR updates the SQL query used for VALIDATE CONSTRAINT for foreign keys.
The new implementation is compatible with the recent changes to FK matching
semantics (both MATCH FULL and MATCH SIMPLE). It also uses a merge join, which
will improve performance significantly compared to the old hash join
implementation.

Release note (sql change): VALIDATE CONSTRAINT for foreign keys is now
compatible with the new MATCH FULL and MATCH SIMPLE semantics, and is more
performant.

Fixes #33452
Co-authored-by: Lucy Zhang <[email protected]>

This PR updates the SQL query used for VALIDATE CONSTRAINT for foreign keys.
The new implementation is compatible with the recent changes to FK matching
semantics (both MATCH FULL and MATCH SIMPLE). It also uses a merge join, which
will improve performance significantly compared to the old hash join
implementation.

Release note (sql change): VALIDATE CONSTRAINT for foreign keys is now
compatible with the new MATCH FULL and MATCH SIMPLE semantics, and is more
performant.

Release note: None

34548: storage: apply lease change side-effects on snapshot recipients r=nvanbenschoten a=nvanbenschoten

Fixes #34025.
Fixes #33624.
Fixes #33335.
Fixes #33151.
Fixes #33149.
Fixes #34159.
Fixes #34293.
Fixes #32813.
Fixes #30886.
Fixes #34228.
Fixes #34321.

It is rare but possible for a replica to become a leaseholder but not learn about this until it applies a snapshot. Immediately upon the snapshot application's `ReplicaState` update, the replica will begin operating as a standard leaseholder.

Before this change, leases acquired in this way would not trigger in-memory side-effects to be performed. This could result in a regression in the new leaseholder's timestamp cache compared to the previous leaseholder's cache, allowing write-skew like we saw in #34025. This could presumably result in other anomalies as well, because all of the steps in `leasePostApply` were skipped (as theorized by https://github.com/cockroachdb/cockroach/issues/34025#issuecomment-460393818).

This PR fixes this bug by detecting lease updates when applying snapshots and making sure to react correctly to them. It also likely fixes the referenced issue. The new test demonstrates that without this fix, the serializable violation speculated about in the issue was possible.
Co-authored-by: Nathan VanBenschoten <[email protected]>

34465: sql: add errors to IndexedRow(s) interfaces r=yuzefovich a=yuzefovich

Adds an option of returning an error for tree.IndexedRow and
tree.IndexedRows methods.

Release note: None
Co-authored-by: Yahor Yuzefovich <[email protected]>

34242: intentresolver: coalesce GCRequests into batches r=ajwerner a=ajwerner

This PR comes in two commits. The first introduces a library for batching individual requests by range ID and sends them based on configured size and time policies, the second adopts this library in the IntentResolver. There is certainly some tuning and cleanup to be done in the first commit but I wanted to get this out there and see how y'all feel about it. The most egregious part is likely the naming.

On the plus side, for a specific and certainly somewhat contrived benchmark using the below command on 3x n1-standard-32's the change yields an appreciable win.
 ```
roachprod run ajwerner-test:4 -- ./workload run kv '{pgurl:1-3}' --init --splits=256 --duration 90s --batch 2 --read-percent=0 --concurrency=2048
```
```
name       old ops/s   new ops/s   delta
KV0     6.23k ± 1%  7.49k ± 2%  +20.12%  (p=0.008 n=5+5)
```

Fixes #33574.
Co-authored-by: Andrew Werner <[email protected]>

importccl: skip TestImportCSVStmt

It's breaking the master build. See #34568.

Release note: None

Also improve a comment.

Release note: None

Adds an option of returning an error for tree.IndexedRow and
tree.IndexedRows methods.

Release note: None

This commit adopts the new Batcher (poorly named and probably the package is
not in the right place) inside of the IntentResolver when sending requests to
GC transaction records. The change also unhooks the GCRequests from the
semaphore which provides backpressure on intent resolution because the batching
leads to intentionally large increases in processing time even if the cluster
were not under load.

Running the below benchmark on 3x n1-standard-32's yields an appreciable win.
```
roachprod run ajwerner-test:4 -- ./workload run kv '{pgurl:1-3}' --init --splits=256 --duration 90s --batch 2 --read-percent=0 --concurrency=2048
```

32 core:
```
name       old ops/s   new ops/s   delta
Cockroach  6.27k ± 1%  8.36k ± 1%  +33.28%  (p=0.008 n=5+5)
```

16 core:
```
name       old ops/s   new ops/s   delta
Cockroach  6.13k ± 1%  6.43k ± 0%  +4.93%  (p=0.008 n=5+5)
```

4 core:
```
name       old ops/s   new ops/s   delta
Cockroach  2.32k ± 1%  2.47k ± 1%  +6.38%  (p=0.008 n=5+5)
```

Fixes #33574

Release note (performance improvement): Batch transaction record gc requests
on a per range basis to reduce the number of raft entries in a high throughput,
write heavy, transactional workload.

Release note: None

Fixes #34025.
Fixes #33624.
Fixes #33335.
Fixes #33151.
Fixes #33149.
Fixes #34159.
Fixes #34293.
Fixes #32813.
Fixes #30886.
Fixes #34228.
Fixes #34321.

It is rare but possible for a replica to become a leaseholder but not
learn about this until it applies a snapshot. Immediately upon the
snapshot application's `ReplicaState` update, the replica will begin
operating as a standard leaseholder.

Before this change, leases acquired in this way would not trigger
in-memory side-effects to be performed. This could result in a regression
in the new leaseholder's timestamp cache compared to the previous
leaseholder, allowing write-skew like we saw in #34025. This could
presumably result in other anomalies as well, because all of the
steps in `leasePostApply` were skipped.

This PR fixes this bug by detecting lease updates when applying
snapshots and making sure to react correctly to them. It also likely
fixes the referenced issue. The new test demonstrated that without
this fix, the serializable violation speculated about in the issue
was possible.

Release note (bug fix): Fix bug where lease transfers passed through
Snapshots could forget to update in-memory state on the new leaseholder,
allowing write-skew between read-modify-write operations.

34562: storage: reduce load on timestamp cache due to PushTxn reqs r=nvanbenschoten a=nvanbenschoten

`PushTxnRequest` uses the timestamp cache to carry information about
pushed transactions that don't have transaction records. Technically
it doesn't even need to update the timestamp cache if a transaction
record exists since it can modify the record directly. Doing so is
harmless though, so we don't do anything tricky. However, one case
where we were probably a little too permissive was with COMMITTED
transactions.

This commit restricts the cases where we update the timestamp cache
to only when the pushed transaction is PENDING or ABORTED. Updating
the read timestamp cache for COMMITTED transactions was fine, but
completely unnecessary and not intentional. Since there is some cost
to updating the timestamp cache, we might as well avoid this case.

Release note: None
Co-authored-by: Nathan VanBenschoten <[email protected]>

34544: sql: use InternalExecutor for validating checks r=lucy-zhang a=lucy-zhang

Use `InternalExecutor` for validating check constraints. The point of this is
to separate out the validation implementation from the planner used for the
ALTER TABLE query, so that `validateCheckExpr` can be called when the
constraint validation moves through the schema changer (to come in a later
change).

Release note: None
Co-authored-by: Lucy Zhang <[email protected]>

This commit introduces a library to group requests by range ID and send them in
batches according to policies based on total time a request has been queued
(wait time), time since the last request for a given range was enqueued (idle
time), as well as the byte size and number of requests in a batch. This initial
approach strives to be as simple as possible and leaves open a large number of
questions about future directions in TODOs.

Release note: None

34576: roachtest: add O-roachtest label to failures r=jordanlewis a=jordanlewis

Release note: None

34578: opt: fix panic due to incorrect null counts r=rytaft a=rytaft

This commit fixes a panic caused by not applying the selectivity
of a `SELECT` operation to the estimated null count for a column,
causing the null count to be higher than the row count. This
panic happens when all values of the column are null.

Fixes #34440

Release note (bug fix): Fixed a panic due to incorrect statistics
calculations when all values of a column are null.
Co-authored-by: Jordan Lewis <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>

34574: jobs: fix potential npe on not-found job r=jordanlewis a=jordanlewis

Fixes #34495.

Release note (bug fix): prevent crash when updating a job that doesn't
exist.
Co-authored-by: Jordan Lewis <[email protected]>

This commit fixes a panic caused by not applying the selectivity
of a SELECT operation to the estimated null count for a column,
causing the null count to be higher than the row count. This
panic happens when all values of the column are null.

Fixes #34440

Release note (bug fix): Fixed a panic due to incorrect statistics
calculations when all values of a column are null.

34550: storage: ensure RangeFeed-watched ranges have a lease r=nvanbenschoten a=danhhz

RangeFeeds can be served from followers, so it doesn't matter which
replica has the lease, just that some replica has it. Otherwise, we
won't get closed timestamp updates for that range, which means the
RangeFeed will not checkpoint any timestamps.

Closes #34390

Release note (bug fix): `CHANGEFEED`s with `changefeed.push.enabled` set
to true now resolve timestamps in the presense of inactive ranges.
Co-authored-by: Daniel Harrison <[email protected]>

Release note: None

Release note (bug fix): prevent crash when updating a job that doesn't
exist.

33478: sql,kv,followerreadsccl: enable follower reads for historical queries r=ajwerner a=ajwerner

Follower reads are reads which can be served from any replica as opposed to just
the current lease holder. The foundation for this change was laid with the work
to introduce closed timestamps and to support follower reads at the replica
level. This change adds the required support to the sql and kv layers and
additionally exposes a new syntax to ease client adoption of the functionality.

The change adds the followerreadsccl package with logic to check when follower
reads are safe and to inject the functionality so that it can be packaged as an
enterprise feature.

Modifies `AS OF SYSTEM TIME` semantics to allow for the evaluation of a new
builtin tentatively called `follower_read_timestamp()` in addition to constant
expressions. This new builtin ensures that an enterprise license exists and then
returns a time that can likely be used to read from a follower.

The change abstracts (and renames to the more appropriate replicaoracle) the
existing leaseHolderOracle in the distsqlplan package to allow a follower read
aware policy to be injected.

Lastly the change add to kv a site to inject a function for checking if follower
reads are safe and allowed given a cluster, settings, and batch request.

This change includes a high level roachtest which validates observable behavior
of performing follower reads by examining latencies for reads in a geo-
replicated setting.

Implements #33474 
Fixes #16593

Release note (enterprise change): Add support for performing sufficiently old
historical reads against closest replicas rather than leaseholders. A new
builtin function `follower_read_timestamp()` which can be used with `AS OF
SYSTEM TIME` clauses to generate a timestamp which is likely to be safe for
reads from a follower.
Co-authored-by: Andrew Werner <[email protected]>

34472: gossip: demonstrate problem with loopback info propagation r=tbg a=petermattis

Release note: None
Co-authored-by: Peter Mattis <[email protected]>

34454: sql: turn on query cache r=andy-kimball a=RaduBerinde

Release note (performance improvement): A query plan cache saves a
portion of the planning time for frequent queries.

Some benchmarks with KV: https://docs.google.com/spreadsheets/d/1n818OsKTWsatbpXo5ddOJ53mL4hY-JhNJEuIlytilLc/edit#gid=0

While there is some improvement, the implicit prepare case is still nowhere close to prepare-once; I plan to benchmark and do more work on the prepare path. I think it's a good idea to enable the cache now though, so we get more time to weed out any problems. 
Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Andrew Kimball <[email protected]>

Do not use cached memo if the SafeUpdates setting does not match.

Release note: None

We no longer need special cases for these two request types in
updateTimestampCache.

Release note: None