sql,kv,followerreadsccl: enable follower reads for historical queries #33478

ajwerner · 2019-01-03T19:16:02Z

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.

cockroach-teamcity · 2019-01-03T19:16:11Z

This change is

nvanbenschoten

Sorry not reviewing this earlier, was waiting for a notification that it was no longer [DNM].

Mostly , though I'm not familiar enough with the SpanResolver stuff to review that alone. @RaduBerinde want to take a look?

Reviewed 17 of 22 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)

pkg/ccl/followerreadsccl/followerreads_test.go, line 38 at r1 (raw file):

func TestEvalFollowerReadOffset(t *testing.T) {
	defer leaktest.AfterTest(t)()
	disableEnterprise := utilccl.TestingEnableEnterprise()

Do we need to defer disableEnterprise() as well, in case one of the Fatals fires? Same question throughout.

pkg/ccl/followerreadsccl/followerreads_test.go, line 46 at r1 (raw file):

	}
	disableEnterprise()
	if _, err := evalFollowerReadOffset(uuid.MakeV4(), st); err == nil {

testutils.IsError

pkg/ccl/followerreadsccl/followerreads_test.go, line 96 at r1 (raw file):

TestFollowerReadMultiple

Consider making this name a bit more specific, like TestFollowerReadMultipleValidation

pkg/cmd/roachtest/follower_reads.go, line 103 at r1 (raw file):

	}
	chooseKV := func() (k int, v int64) {
		for k, v = range data {

Is the goal to randomize the kv? If so, let's leave a comment.

pkg/cmd/roachtest/follower_reads.go, line 165 at r1 (raw file):

	}
	// Wait for follower_timestamp() historical reads to have data.
	// TODO(ajwerner): introspect the database to determine how long to wait.

We you planning on addressing this? Based on kv.closed_timestamp.target_duration?

pkg/cmd/roachtest/follower_reads.go, line 224 at r1 (raw file):

// percentile remain below target latency between start and end ignoring the
// first 20s.
func verifySQLLatency(

This is really nice.

pkg/cmd/roachtest/follower_reads.go, line 265 at r1 (raw file):

const followerReadsMetric = "follower_reads_success_count"

// getFollowerReadCounts returns a slice from node to

Looks like this was missed.

pkg/sql/sem/builtins/builtins.go, line 1741 at r1 (raw file):

	),

	tree.FollowerReadTimestampFunctionName: makeBuiltin(

Let's add a few logic tests that call this function directly.

pkg/sql/sem/tree/as_of.go, line 63 at r1 (raw file):

		}
		if te, err = fe.TypeCheck(semaCtx, types.TimestampTZ); err != nil {
			// TODO(ajwerner): should this be a panic?

Users can pass whatever Expr they want and it won't be caught until here, right? So I don't think we want to panic.

pkg/storage/metrics.go, line 284 at r1 (raw file):

	// Metric for tracking follower reads.
	metaFollowerReadsCount = metric.Metadata{
		Name:        "follower_reads.success_count",

Heh, I was looking for this a few days ago.

pkg/storage/replica_read.go, line 129 at r1 (raw file):

	} else {
		log.Event(ctx, "read completed")
		if isFollowerRead {

Why wait to update this?

nvanbenschoten · 2019-02-04T23:18:52Z

Actually, @RaduBerinde's on vacation. @jordanlewis graciously said he'd take a look.

ajwerner

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

pkg/ccl/followerreadsccl/followerreads_test.go, line 38 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we need to defer disableEnterprise() as well, in case one of the Fatals fires? Same question throughout.

Done.

pkg/ccl/followerreadsccl/followerreads_test.go, line 46 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

testutils.IsError

Done.

pkg/ccl/followerreadsccl/followerreads_test.go, line 96 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

TestFollowerReadMultiple
Consider making this name a bit more specific, like TestFollowerReadMultipleValidation

Done.

pkg/cmd/roachtest/follower_reads.go, line 103 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is the goal to randomize the kv? If so, let's leave a comment.

Done.

pkg/cmd/roachtest/follower_reads.go, line 165 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We you planning on addressing this? Based on kv.closed_timestamp.target_duration?

One wrinkle in doing that is that we don't allow reading hidden cluster setting through crdb_internal.cluster_settings. I set it to read target_duration and close_fraction but still consts the target_multiple.

pkg/sql/sem/builtins/builtins.go, line 1741 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's add a few logic tests that call this function directly.

Seems like logictests run without the ccl code. I added a test that calls this function and errors out.

pkg/sql/sem/tree/as_of.go, line 63 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Users can pass whatever Expr they want and it won't be caught until here, right? So I don't think we want to panic.

well no, once we're here we know that the fe is a *FuncExpr and that the resolved function defintiion has the name follower_read_timestamp so the only case that this TypeCheck would fail would be if somehow the builtin function of that name is doesn't return a TimestampTZ (which is the only thing it does currently return). Essentially it'd have to be a programmer error, hence the panic.

pkg/storage/replica_read.go, line 129 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why wait to update this?

In case the follower read fails? I'm happy to move it up.

danhhz

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @jordanlewis, and @nvanbenschoten)

pkg/sql/sem/builtins/builtins.go, line 1741 at r1 (raw file):

Previously, ajwerner wrote…

Seems like logictests run without the ccl code. I added a test that calls this function and errors out.

There are ccl logic tests as well. Look for logictestccl and make testccllogic

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @danhhz, @jordanlewis, and @nvanbenschoten)

pkg/sql/sem/builtins/builtins.go, line 1741 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

There are ccl logic tests as well. Look for logictestccl and make testccllogic

Thanks! added one.

ajwerner

@knz I'm not actively ignoring your concerns about the function naming, I'm not sure how to balance the desire to get this in to the current alpha so that we can include this feature in the next release with the desire to have enough discussion such that stakeholders are satisfied with the decision-making process. I remain open to continuing the discussion and improving the name for the release.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @danhhz, @jordanlewis, and @nvanbenschoten)

nvanbenschoten

Reviewed 1 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @jordanlewis, and @nvanbenschoten)

pkg/cmd/roachtest/follower_reads.go, line 165 at r1 (raw file):

Previously, ajwerner wrote…

One wrinkle in doing that is that we don't allow reading hidden cluster setting through crdb_internal.cluster_settings. I set it to read target_duration and close_fraction but still consts the target_multiple.

👍

pkg/sql/sem/tree/as_of.go, line 63 at r1 (raw file):

Previously, ajwerner wrote…

well no, once we're here we know that the fe is a *FuncExpr and that the resolved function defintiion has the name follower_read_timestamp so the only case that this TypeCheck would fail would be if somehow the builtin function of that name is doesn't return a TimestampTZ (which is the only thing it does currently return). Essentially it'd have to be a programmer error, hence the panic.

I don't think I made my point very clearly - I'm referring to argument expressions passed to the function. TypeCheck is exactly what guarantees that a function overload is found with parameters compatible with the provided arguments. If no compatible overload is found, overload resolution will fail and we'll get an error from TypeCheck.

For instance, the following would panic here:

select * from t as of system time follower_read_timestamp('boom');

pkg/storage/replica_read.go, line 129 at r1 (raw file):

Previously, ajwerner wrote…

In case the follower read fails? I'm happy to move it up.

Yeah, I'd move it up. Even follower read that hits an error (e.g. a WriteIntentError) should be counted.

knz · 2019-02-05T01:28:39Z

At least throw an `experimental_` in there so users understand its not prod ready yet. :-) -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

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` the semantics to allow for the evaluation of a new builtin tentatively called `experimental_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. Release note (enterprise change): Add support for performing sufficiently old historical reads against closest replicas rather than leaseholders. A new builtin function `experimental_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.

ajwerner

Done.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis and @nvanbenschoten)

pkg/sql/sem/tree/as_of.go, line 63 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I don't think I made my point very clearly - I'm referring to argument expressions passed to the function. TypeCheck is exactly what guarantees that a function overload is found with parameters compatible with the provided arguments. If no compatible overload is found, overload resolution will fail and we'll get an error from TypeCheck.

For instance, the following would panic here:
select * from t as of system time follower_read_timestamp('boom');

Cool. Removed the TODO and added a logic test.

pkg/storage/replica_read.go, line 129 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Yeah, I'd move it up. Even follower read that hits an error (e.g. a WriteIntentError) should be counted.

Done.

nvanbenschoten

mod SpanResolver stuff.

Reviewed 4 of 6 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @nvanbenschoten)

jordanlewis

- as I understand the SpanResolver stuff just got a refactor and a tiny new "closest" policy, more or less, right Andrew? Thanks for the cleanup!

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, and @nvanbenschoten)

pkg/sql/distsqlplan/replicaoracle/oracle.go, line 148 at r2 (raw file):

type closestOracle struct {
	gossip      *gossip.Gossip

This is the only new code in the file, correct? Tricky to review code when a big move happened at the same time as an addition to the that was added :)

ajwerner

Yes that’s pretty much correct.

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @jordanlewis and @nvanbenschoten)

pkg/sql/distsqlplan/replicaoracle/oracle.go, line 148 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

This is the only new code in the file, correct? Tricky to review code when a big move happened at the same time as an addition to the that was added :)

Sorry about that. This diff was written in a very piecemeal way. I definitely told have split it off in to a couple of commits. TFTR!

ajwerner · 2019-02-05T14:53:53Z

bors r+

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]>

craig · 2019-02-05T15:37:56Z

Build succeeded

GitHub CI (Cockroach)

ajwerner requested review from nvanbenschoten and a team January 3, 2019 19:16

ajwerner force-pushed the ajwerner/follower-reads-impl branch 6 times, most recently from b88560a to 8f08c2c Compare January 9, 2019 15:29

ajwerner force-pushed the ajwerner/follower-reads-impl branch from 8f08c2c to cb88d2b Compare February 1, 2019 00:34

ajwerner changed the title ~~[DNM] sql,kv,followerreadsccl: enable follower reads for historical queries~~ sql,kv,followerreadsccl: enable follower reads for historical queries Feb 1, 2019

ajwerner force-pushed the ajwerner/follower-reads-impl branch 4 times, most recently from 5838fee to 1b6e1d5 Compare February 4, 2019 19:32

nvanbenschoten reviewed Feb 4, 2019

View reviewed changes

ajwerner force-pushed the ajwerner/follower-reads-impl branch from 1b6e1d5 to 62e9c64 Compare February 5, 2019 00:09

ajwerner commented Feb 5, 2019

View reviewed changes

ajwerner requested a review from jordanlewis February 5, 2019 00:10

danhhz reviewed Feb 5, 2019

View reviewed changes

ajwerner force-pushed the ajwerner/follower-reads-impl branch 2 times, most recently from 7b960bd to 52274fe Compare February 5, 2019 00:47

ajwerner commented Feb 5, 2019

View reviewed changes

nvanbenschoten reviewed Feb 5, 2019

View reviewed changes

ajwerner force-pushed the ajwerner/follower-reads-impl branch from 52274fe to 70be833 Compare February 5, 2019 06:34

ajwerner commented Feb 5, 2019

View reviewed changes

nvanbenschoten approved these changes Feb 5, 2019

View reviewed changes

jordanlewis approved these changes Feb 5, 2019

View reviewed changes

ajwerner commented Feb 5, 2019

View reviewed changes

craig bot merged commit 70be833 into cockroachdb:master Feb 5, 2019

jseldess mentioned this pull request Feb 12, 2019

sql,kv,followerreadsccl: enable follower reads for historical queries cockroachdb/docs#4360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql,kv,followerreadsccl: enable follower reads for historical queries #33478

sql,kv,followerreadsccl: enable follower reads for historical queries #33478

ajwerner commented Jan 3, 2019 •

edited

Loading

cockroach-teamcity commented Jan 3, 2019

nvanbenschoten left a comment

nvanbenschoten commented Feb 4, 2019

ajwerner left a comment

danhhz left a comment

ajwerner left a comment

ajwerner left a comment

nvanbenschoten left a comment

knz commented Feb 5, 2019 via email

ajwerner left a comment

nvanbenschoten left a comment

jordanlewis left a comment

ajwerner left a comment

ajwerner commented Feb 5, 2019

craig bot commented Feb 5, 2019

sql,kv,followerreadsccl: enable follower reads for historical queries #33478

sql,kv,followerreadsccl: enable follower reads for historical queries #33478

Conversation

ajwerner commented Jan 3, 2019 • edited Loading

cockroach-teamcity commented Jan 3, 2019

nvanbenschoten left a comment

Choose a reason for hiding this comment

nvanbenschoten commented Feb 4, 2019

ajwerner left a comment

Choose a reason for hiding this comment

danhhz left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

knz commented Feb 5, 2019 via email

ajwerner left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner commented Feb 5, 2019

craig bot commented Feb 5, 2019

Build succeeded

ajwerner commented Jan 3, 2019 •

edited

Loading