Skip to content

storage/rangefeed: push rangefeed.Filter into Raft processing goroutine#41788

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/rangefeedFilter
Nov 21, 2019
Merged

storage/rangefeed: push rangefeed.Filter into Raft processing goroutine#41788
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/rangefeedFilter

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Oct 21, 2019

This commit creates a rangefeed.Filter object, which informs the producer of logical operations of the information that a rangefeed.Processor is interested in, given its current set of registrations. It can be used to avoid performing extra work to provide the Processor with information which will be ignored.

This is an important optimization for single-key or small key span rangefeeds, as it avoids some extraneous work for the key spans not being watched.

For now, this extra work is just fetching the post-operation value for MVCCWriteValueOp and MVCCCommitIntentOp operations, but this can be extended in the future to include the pre-operation value as well. This will be important to avoid any perf regression when addressing #28666.

@nvb nvb requested a review from danhhz October 21, 2019 22:05
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

nvb added a commit to nvb/cockroach that referenced this pull request Oct 22, 2019
Fixes cockroachdb#28666.
First commit from cockroachdb#41788.

This commit adds a `WITH diff` option to CREATE CHANGEFEED. When the
option is provided, changefeeds publications will include a `before`
field, which includes the value of the row before the CDC update was
applied.

We are able to implement this efficiently by pushing the option into
Rangefeed and performing the scan of the previous value immediately
before applying the update in the Raft processing goroutine. cockroachdb#41788
allows us to avoid performing this lookup when `WITH diff` isn't
specified for Rangefeeds, so the small (unmeasured) perf hit is
strictly opt-in.

DNM: this still needs testing and a bit of polish. It's also not clear
how we want to handle catch-up scans when the option is provided.
nvb added a commit to nvb/cockroach that referenced this pull request Nov 16, 2019
Fixes cockroachdb#28666.
First commit from cockroachdb#41788.

This commit adds a `WITH diff` option to CREATE CHANGEFEED. When the
option is provided, changefeeds publications will include a `before`
field, which includes the value of the row before the CDC update was
applied.

We are able to implement this efficiently by pushing the option into
Rangefeed and performing the scan of the previous value immediately
before applying the update in the Raft processing goroutine. cockroachdb#41788
allows us to avoid performing this lookup when `WITH diff` isn't
specified for Rangefeeds, so the small (unmeasured) perf hit is
strictly opt-in.

Release note (sql change): CHANGEFEED now supports a `WITH diff` option,
which instructs it to include a `before` field in each publication.
Copy link
Copy Markdown
Contributor

@danhhz danhhz left a comment

Choose a reason for hiding this comment

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

:lgtm: (assuming this is still the same as it was in #41793)

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @danhhz)

This commit creates a rangefeed.Filter object, which informs the producer of
logical operations of the information that a rangefeed Processor is interested
in, given its current set of registrations. It can be used to avoid performing
extra work to provide the Processor with information which will be ignored.

This is an important optimization for single-key or small key span rangefeeds,
as it avoids some extraneous work for the key spans not being watched.

For now, this extra work is just fetching the post-operation value for
MVCCWriteValueOp and MVCCCommitIntentOp operations, but this can be extended in
the future to include the pre-operation value as well. This will be important
to avoid any perf regression when addressing cockroachdb#28666.
@nvb nvb force-pushed the nvanbenschoten/rangefeedFilter branch from 59a4c2b to acf26c0 Compare November 20, 2019 23:27
@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Nov 20, 2019

assuming this is still the same as it was in #41793

Yes, exactly the same. TFTR!

bors r=danhhz

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 20, 2019

Build failed (retrying...)

craig bot pushed a commit that referenced this pull request Nov 21, 2019
41788: storage/rangefeed: push rangefeed.Filter into Raft processing goroutine r=danhhz a=nvanbenschoten

This commit creates a `rangefeed.Filter` object, which informs the producer of logical operations of the information that a `rangefeed.Processor` is interested in, given its current set of registrations. It can be used to avoid performing extra work to provide the Processor with information which will be ignored.

This is an important optimization for single-key or small key span rangefeeds, as it avoids some extraneous work for the key spans not being watched.

For now, this extra work is just fetching the post-operation value for `MVCCWriteValueOp` and `MVCCCommitIntentOp` operations, but this can be extended in the future to include the pre-operation value as well. This will be important to avoid any perf regression when addressing #28666.

42580: sql: add precision support for TIMESTAMP/TIMESTAMPTZ (v2) r=otan a=otan

Refs #32098
Alternative to #42552

----

Add precision support for TIMESTAMP/TIMESTAMPTZ, supporting precisions
from 0 to 6 inclusive.

When storing this in the type proto, we introduce a `TimePrecisionIsSet`
variable, which is used to differentiate 0 (unset) or 0 (explicitly set)
for time related protos. This is abstracted away in the `Precision()`
function which returns the correct precision (unset => 6) for time. This
allows forwards and backwards compatibility.

Note Time will come later, as it involves a bit more plumbing to the
base library.

Release note (sql change): Introduces precision support for TIMESTAMP
and TIMESTAMPTZ, supporting precisions from 0 to 6 inclusive. Previous
versions of TIMESTAMP and TIMESTAMPTZ will default to 6 units of
precision. Note that if anyone downgrades whilst having precision set,
they will have full precision (6) again, but if they re-upgrade they
will find their precisions truncated again.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 21, 2019

Build succeeded

@craig craig bot merged commit acf26c0 into cockroachdb:master Nov 21, 2019
nvb added a commit to nvb/cockroach that referenced this pull request Nov 21, 2019
Fixes cockroachdb#28666.
First commit from cockroachdb#41788.

This commit adds a `WITH diff` option to CREATE CHANGEFEED. When the
option is provided, changefeeds publications will include a `before`
field, which includes the value of the row before the CDC update was
applied.

We are able to implement this efficiently by pushing the option into
Rangefeed and performing the scan of the previous value immediately
before applying the update in the Raft processing goroutine. cockroachdb#41788
allows us to avoid performing this lookup when `WITH diff` isn't
specified for Rangefeeds, so the small (unmeasured) perf hit is
strictly opt-in.

Release note (sql change): CHANGEFEED now supports a `WITH diff` option,
which instructs it to include a `before` field in each publication.
craig bot pushed a commit that referenced this pull request Nov 21, 2019
41793: ccl/changefeedccl: implement `WITH diff` option r=nvanbenschoten a=nvanbenschoten

Fixes #28666.

This commit adds a `WITH diff` option to CREATE CHANGEFEED. When the option is provided, changefeeds publications will include a `before` field, which includes the value of the row before the CDC update was applied.

We are able to implement this efficiently by pushing the option into Rangefeed and performing the scan of the previous value immediately before applying the update in the Raft processing goroutine. #41788 allows us to avoid performing this lookup when `WITH diff` isn't specified for Rangefeeds, so the small (unmeasured) perf hit is strictly opt-in.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@nvb nvb deleted the nvanbenschoten/rangefeedFilter branch December 27, 2019 22:59
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.

3 participants