Skip to content

storage: rename SSTSnapshotStorage{,Scratch,File} vars#43980

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
irfansharif:200114.sss-rename
Jan 14, 2020
Merged

storage: rename SSTSnapshotStorage{,Scratch,File} vars#43980
craig[bot] merged 1 commit intocockroachdb:masterfrom
irfansharif:200114.sss-rename

Conversation

@irfansharif
Copy link
Copy Markdown
Contributor

Around snapshot receiving code, we have variables for SSTSnapshotStorage, SSTSnapshotStorageScratch, and SSTSnapshotStorageFile types
all abbreviated to some variation of sss{,s,f}. We also have "ssts" to
talk about SSTs. This is all terribly confusing, we could do with some
much needed clarity here.

Release note: None.

@irfansharif irfansharif requested review from ajwerner and nvb January 14, 2020 20:18
Around snapshot receiving code, we have variables for
SSTSnapshotStorage, SSTSnapshotStorageScratch, and
SSTSnapshotStorageFile types all abbreviated to some variation of
sss{,s,f}. We also have "ssts" to talk about SSTs. This is all terribly
confusing, we could do with some much needed clarity here.

Release note: None.
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

@irfansharif
Copy link
Copy Markdown
Contributor Author

bors r=ajwerner

craig bot pushed a commit that referenced this pull request Jan 14, 2020
42845: coldata: expose a modifier for batchSize variable r=yuzefovich a=yuzefovich

This commit exposes a setter for batchSize private variable to be
modified in tests and runs all tests in sql/colexec with a random
batch size in [3, 4096] range.

The tests in several places needed to be adjusted because their
assumptions might no longer hold when batch size is modified.

Fixes: #40791.

Release note: None

43409: execinfrapb: print input types one per line in EXPLAIN (DISTSQL, TYPES) r=yuzefovich a=yuzefovich

Previously, input types were printed in a single line within the input
synchronizer box on the flow diagram. However, when we have a processor
with two inputs and when each input has several columns, the boxes would
collide and make the types unreadable. This commit changes it to print
each type one per line, so there is no collision between different boxes
of input synchronizer. Separately, cockroachdb.github.io has been
adjusted so that the boxes for the input synchronizers could be
displayed well in such scenario.

Release note: None

43887: pkg/sql/opt: validate correct use of FOR UPDATE locking clauses r=nvanbenschoten a=nvanbenschoten

The second commit updates our SQL grammar to be identical to Postgres regarding select statement locking clauses. Concretely, this results in the following changes in behavior:
- We can now parse multiple locking clause items
- We can now parse the `FOR READ ONLY` syntax, which is a no-op
- We can now parse the `SELECT ... FOR UPDATE ... LIMIT` syntax

Ref: https://github.com/postgres/postgres/blob/1a4a0329650b0545a54afb3c317aa289fd817f8a/src/backend/parser/gram.y#L11834

The third commit introduces a number of validation checks into the optbuilder to ensure that FOR UPDATE locking clauses are only being used in places where they're allowed. Most of this was based off of Postgres, which disallows the same set of operations to be used with `FOR [KEY] UPDATE/SHARE` locking.

Ref: https://github.com/postgres/postgres/blob/1a4a0329650b0545a54afb3c317aa289fd817f8a/src/backend/parser/analyze.c#L2691

Release note (sql change): Invalid usages of FOR UPDATE locking clauses are now rejected by the SQL optimizer.

43980: storage: rename SSTSnapshotStorage{,Scratch,File} vars r=ajwerner a=irfansharif

Around snapshot receiving code, we have variables for SSTSnapshotStorage, SSTSnapshotStorageScratch, and SSTSnapshotStorageFile types
all abbreviated to some variation of sss{,s,f}. We also have "ssts" to
talk about SSTs. This is all terribly confusing, we could do with some
much needed clarity here.

Release note: None.

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Copy link
Copy Markdown
Contributor

@nvb nvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r1.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 14, 2020

Build succeeded

@craig craig bot merged commit 5b64ca7 into cockroachdb:master Jan 14, 2020
@irfansharif irfansharif deleted the 200114.sss-rename branch April 10, 2020 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants