Skip to content

Commit

Permalink
Merge cockroachdb#100533 cockroachdb#100776 cockroachdb#101076 cockro…
Browse files Browse the repository at this point in the history 
…achdb#101078

100533: workload: jitter the teardown of connections to prevent thundering herd r=sean- a=sean-

This change upgrades workload's use of pgx from v4 to v5 in order to allow jittering the teardown of connections.  This change sets a max connection age of 5min and jitters the teardown by 30s.  Upgrading to pgx v5 also adds non-blocking pgxpool connection acquisition.

workload: add flags to manage the age and lifecycle of connection pool

Add flags to all workload types to specify:

* the max connection age: `--max-conn-lifetime duration`
* the max connection age jitter: `--max-conn-lifetime-jitter duration`
* the max connection idle time: `--max-conn-idle-time duration`
* the connection health check interval: `--conn-healthcheck-period duration`
* the min number of connections in the pool: `--min-conns int`

workload: add support for remaining pgx query modes

Add support for pgx.QueryExecModeCacheDescribe and pgx.QueryExecModeDescribeExec.  Previously, only three of the five query modes were available.

workload: fix race condition when recording histogram data

Release note (cli change): workload jitters teardown of connections to prevent thundering herd impacting P99 latency results.

Release note (cli change): workload utility now has flags to tune the connection pool used for testing.  See `--conn-healthcheck-period`, `--min-conns`, and the `--max-conn-*` flags for details.

Release note (cli change): workload now supports every [PostgreSQL query mode](https://github.com/jackc/pgx/blob/fa5fbed497bc75acee05c1667a8760ce0d634cba/conn.go#L167-L182) available via the underlying pgx driver.

100776: opt: fix ordering-related optimizer panics r=DrewKimball a=DrewKimball

It is possible for some functional-dependency information to be visible to a child operator but invisible to its parent. This could previously cause panics when a child provided an ordering that could be proven to satisfy the required ordering with the child FDs, but not with the parent's FDs.

This patch adds a step to the logic that builds provided orderings that ensures a provided ordering can be proven to respect the required ordering without needing additional FD information. This ensures that a parent never needs to know its child's FDs in order to prove that the provided ordering is correct. The extra step is a no-op in the common case when the provided ordering can already be proven to respect the required ordering.

Informs cockroachdb#85393
Informs cockroachdb#87806
Fixes cockroachdb#96288

Release note (bug fix): Fixed a rare internal error in the optimizer that has existed since before version 22.1, which could occur while enforcing orderings between SQL operators.

101076: autoconfig: prevent a data race in TestAutoConfig r=adityamaru a=knz

Needed for cockroachdb#101069.

The calls to Peek and Pop can run concurrently.

Release note: None
Epic: CRDB-23559

101078: roachtest: move copyfrom test suite to SQL Queries r=srosenberg a=nvanbenschoten

See https://cockroachlabs.slack.com/archives/C0168LW5THS/p1679508254391039.

Epic: None
Release note: None

Co-authored-by: Sean Chittenden <[email protected]>
Co-authored-by: Drew Kimball <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
  • Loading branch information
@sean- @DrewKimball
@knz @nvanbenschoten
5 people committed Apr 10, 2023
5 parents dd237e6 + bdf3f62 + b9b8da6 + 1654c7d + 51ace65 commit 668ec8d
Show file tree
Hide file tree
Showing 38 changed files with 520 additions and 272 deletions.
18 changes: 9 additions & 9 deletions DEPS.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4809,10 +4809,10 @@ def go_deps():
name = "com_github_jackc_pgservicefile",
build_file_proto_mode = "disable_global",
importpath = "github.com/jackc/pgservicefile",
sha256 = "8422a25b9d2b0be05c66ee1ccfdbaab144ce98f1ac678bc647064c560d4cd6e2",
strip_prefix = "github.com/jackc/[email protected]20200714003250-2b9c44734f2b",
sha256 = "1f8bdf75b2a0d750e56c2a94b1d1b0b5be4b29d6df056aebd997162c29bfd8ab",
strip_prefix = "github.com/jackc/[email protected]20221227161230-091c0ba34f0a",
urls = [
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgservicefile/com_github_jackc_pgservicefile-v0.0.0-20200714003250-2b9c44734f2b.zip",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgservicefile/com_github_jackc_pgservicefile-v0.0.0-20221227161230-091c0ba34f0a.zip",
],
)
go_repository(
Expand All @@ -4839,10 +4839,10 @@ def go_deps():
name = "com_github_jackc_pgx_v5",
build_file_proto_mode = "disable_global",
importpath = "github.com/jackc/pgx/v5",
sha256 = "e05b4284fb33e5c0c648b269070dedac6759711c411283177261228ab684f45f",
strip_prefix = "github.com/jackc/pgx/v5@v5.2.0",
sha256 = "e2f4a98f6b8716a6854d0a910c12c3527d35ff78ec5f2d16bf49f85601071bf0",
strip_prefix = "github.com/jackc/pgx/v5@v5.3.1",
urls = [
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgx/v5/com_github_jackc_pgx_v5-v5.2.0.zip",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgx/v5/com_github_jackc_pgx_v5-v5.3.1.zip",
],
)
go_repository(
Expand All @@ -4859,10 +4859,10 @@ def go_deps():
name = "com_github_jackc_puddle_v2",
build_file_proto_mode = "disable_global",
importpath = "github.com/jackc/puddle/v2",
sha256 = "73ea72b52d0a680442d535cf5d9a9713cb0803929c0b4a8e553eda47ee217c44",
strip_prefix = "github.com/jackc/puddle/v2@v2.1.2",
sha256 = "b99ea95df0c0298caf2be786c9eba511bfde2046eccfaa06e89b3e460ab406b0",
strip_prefix = "github.com/jackc/puddle/v2@v2.2.0",
urls = [
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/puddle/v2/com_github_jackc_puddle_v2-v2.1.2.zip",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/puddle/v2/com_github_jackc_puddle_v2-v2.2.0.zip",
],
)
go_repository(
Expand Down
6 changes: 3 additions & 3 deletions build/bazelutil/distdir_files.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,12 +631,12 @@ DISTDIR_FILES = {
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgpassfile/com_github_jackc_pgpassfile-v1.0.0.zip": "1cc79fb0b80f54b568afd3f4648dd1c349f746ad7c379df8d7f9e0eb1cac938b",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgproto3/com_github_jackc_pgproto3-v1.1.0.zip": "e3766bee50ed74e49a067b2c4797a2c69015cf104bf3f3624cd483a9e940b4ee",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgproto3/v2/com_github_jackc_pgproto3_v2-v2.3.1.zip": "57884e299825af31fd01268659f1e671883b73b708a51230da14d6f8ee0e4e36",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgservicefile/com_github_jackc_pgservicefile-v0.0.0-20200714003250-2b9c44734f2b.zip": "8422a25b9d2b0be05c66ee1ccfdbaab144ce98f1ac678bc647064c560d4cd6e2",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgservicefile/com_github_jackc_pgservicefile-v0.0.0-20221227161230-091c0ba34f0a.zip": "1f8bdf75b2a0d750e56c2a94b1d1b0b5be4b29d6df056aebd997162c29bfd8ab",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgtype/com_github_jackc_pgtype-v1.11.0.zip": "6a257b81c0bd386d6241219a14ebd41d574a02aeaeb3942670c06441b864dcad",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgx/v4/com_github_jackc_pgx_v4-v4.16.1.zip": "c3a169a68ff0e56f9f81eee4de4d2fd2a5ec7f4d6be159159325f4863c80bd10",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgx/v5/com_github_jackc_pgx_v5-v5.2.0.zip": "e05b4284fb33e5c0c648b269070dedac6759711c411283177261228ab684f45f",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/pgx/v5/com_github_jackc_pgx_v5-v5.3.1.zip": "e2f4a98f6b8716a6854d0a910c12c3527d35ff78ec5f2d16bf49f85601071bf0",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/puddle/com_github_jackc_puddle-v1.2.1.zip": "40d73550686666eb1f6df02b65008b2a4c98cfed1254dc4866e6ebe95fbc5c95",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/puddle/v2/com_github_jackc_puddle_v2-v2.1.2.zip": "73ea72b52d0a680442d535cf5d9a9713cb0803929c0b4a8e553eda47ee217c44",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jackc/puddle/v2/com_github_jackc_puddle_v2-v2.2.0.zip": "b99ea95df0c0298caf2be786c9eba511bfde2046eccfaa06e89b3e460ab406b0",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jaegertracing/jaeger/com_github_jaegertracing_jaeger-v1.18.1.zip": "256a95b2a52a66494aca6d354224bb450ff38ce3ea1890af46a7c8dc39203891",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jcmturner/aescts/v2/com_github_jcmturner_aescts_v2-v2.0.0.zip": "717a211ad4aac248cf33cadde73059c13f8e9462123a0ab2fed5c5e61f7739d7",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/jcmturner/dnsutils/v2/com_github_jcmturner_dnsutils_v2-v2.0.0.zip": "f9188186b672e547cfaef66107aa62d65054c5d4f10d4dcd1ff157d6bf8c275d",
Expand Down
4 changes: 3 additions & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ require (
github.com/jackc/pgio v1.0.0 // indirect
github.com/jackc/pgpassfile v1.0.0 // indirect
github.com/jackc/pgproto3/v2 v2.3.1
github.com/jackc/pgservicefile v0.0.0-20200714003250-2b9c44734f2b // indirect
github.com/jackc/pgservicefile v0.0.0-20221227161230-091c0ba34f0a // indirect
github.com/jackc/pgtype v1.11.0
github.com/jackc/pgx/v4 v4.16.1
github.com/jackc/puddle v1.2.1 // indirect
Expand Down Expand Up @@ -156,6 +156,7 @@ require (
github.com/grpc-ecosystem/grpc-gateway v1.16.0
github.com/guptarohit/asciigraph v0.5.5
github.com/irfansharif/recorder v0.0.0-20211218081646-a21b46510fd6
github.com/jackc/pgx/v5 v5.3.1
github.com/jaegertracing/jaeger v1.18.1
github.com/jordan-wright/email v4.0.1-0.20210109023952-943e75fe5223+incompatible
github.com/jordanlewis/gcassert v0.0.0-20221027203946-81f097ad35a0
Expand Down Expand Up @@ -314,6 +315,7 @@ require (
github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639 // indirect
github.com/imdario/mergo v0.3.13 // indirect
github.com/inconshreveable/mousetrap v1.0.0 // indirect
github.com/jackc/puddle/v2 v2.2.0 // indirect
github.com/jcmturner/aescts/v2 v2.0.0 // indirect
github.com/jcmturner/dnsutils/v2 v2.0.0 // indirect
github.com/jcmturner/gofork v1.7.6 // indirect
Expand Down
7 changes: 6 additions & 1 deletion go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1377,8 +1377,9 @@ github.com/jackc/pgproto3/v2 v2.1.1/go.mod h1:WfJCnwN3HIg9Ish/j3sgWXnAfK8A9Y0bwX
github.com/jackc/pgproto3/v2 v2.3.0/go.mod h1:WfJCnwN3HIg9Ish/j3sgWXnAfK8A9Y0bwXYU5xKaEdA=
github.com/jackc/pgproto3/v2 v2.3.1 h1:nwj7qwf0S+Q7ISFfBndqeLwSwxs+4DPsbRFjECT1Y4Y=
github.com/jackc/pgproto3/v2 v2.3.1/go.mod h1:WfJCnwN3HIg9Ish/j3sgWXnAfK8A9Y0bwXYU5xKaEdA=
github.com/jackc/pgservicefile v0.0.0-20200714003250-2b9c44734f2b h1:C8S2+VttkHFdOOCXJe+YGfa4vHYwlt4Zx+IVXQ97jYg=
github.com/jackc/pgservicefile v0.0.0-20200714003250-2b9c44734f2b/go.mod h1:vsD4gTJCa9TptPL8sPkXrLZ+hDuNrZCnj29CQpr4X1E=
github.com/jackc/pgservicefile v0.0.0-20221227161230-091c0ba34f0a h1:bbPeKD0xmW/Y25WS6cokEszi5g+S0QxI/d45PkRi7Nk=
github.com/jackc/pgservicefile v0.0.0-20221227161230-091c0ba34f0a/go.mod h1:5TJZWKEWniPve33vlWYSoGYefn3gLQRzjfDlhSJ9ZKM=
github.com/jackc/pgtype v0.0.0-20190421001408-4ed0de4755e0/go.mod h1:hdSHsc1V01CGwFsrv11mJRHWJ6aifDLfdV3aVjFF0zg=
github.com/jackc/pgtype v0.0.0-20190824184912-ab885b375b90/go.mod h1:KcahbBH1nCMSo2DXpzsoWOAfFkdEtEJpPbVLq8eE+mc=
github.com/jackc/pgtype v0.0.0-20190828014616-a8802b16cc59/go.mod h1:MWlu30kVJrUS8lot6TQqcg7mtthZ9T0EoIBFiJcmcyw=
Expand All @@ -1391,11 +1392,15 @@ github.com/jackc/pgx/v4 v4.0.0-pre1.0.20190824185557-6972a5742186/go.mod h1:X+GQ
github.com/jackc/pgx/v4 v4.12.1-0.20210724153913-640aa07df17c/go.mod h1:1QD0+tgSXP7iUjYm9C1NxKhny7lq6ee99u/z+IHFcgs=
github.com/jackc/pgx/v4 v4.16.1 h1:JzTglcal01DrghUqt+PmzWsZx/Yh7SC/CTQmSBMTd0Y=
github.com/jackc/pgx/v4 v4.16.1/go.mod h1:SIhx0D5hoADaiXZVyv+3gSm3LCIIINTVO0PficsvWGQ=
github.com/jackc/pgx/v5 v5.3.1 h1:Fcr8QJ1ZeLi5zsPZqQeUZhNhxfkkKBOgJuYkJHoBOtU=
github.com/jackc/pgx/v5 v5.3.1/go.mod h1:t3JDKnCBlYIc0ewLF0Q7B8MXmoIaBOZj/ic7iHozM/8=
github.com/jackc/puddle v0.0.0-20190413234325-e4ced69a3a2b/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk=
github.com/jackc/puddle v0.0.0-20190608224051-11cab39313c9/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk=
github.com/jackc/puddle v1.1.3/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk=
github.com/jackc/puddle v1.2.1 h1:gI8os0wpRXFd4FiAY2dWiqRK037tjj3t7rKFeO4X5iw=
github.com/jackc/puddle v1.2.1/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk=
github.com/jackc/puddle/v2 v2.2.0 h1:RdcDk92EJBuBS55nQMMYFXTxwstHug4jkhT5pq8VxPk=
github.com/jackc/puddle/v2 v2.2.0/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4=
github.com/jaegertracing/jaeger v1.18.1 h1:eFqjEpTKq2FfiZ/YX53oxeCePdIZyWvDfXaTAGj0r5E=
github.com/jaegertracing/jaeger v1.18.1/go.mod h1:WRzMFH62rje1VgbShlgk6UbWUNoo08uFFvs/x50aZKk=
github.com/jcmturner/aescts/v2 v2.0.0 h1:9YKLH6ey7H4eDBXW8khjYslgyqG2xZikXP0EQFKrle8=
Expand Down
6 changes: 3 additions & 3 deletions pkg/cmd/roachtest/tests/copyfrom.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,23 +145,23 @@ func registerCopyFrom(r registry.Registry) {
tc := tc
r.Add(registry.TestSpec{
Name: fmt.Sprintf("copyfrom/crdb-atomic/sf=%d/nodes=%d", tc.sf, tc.nodes),
Owner: registry.OwnerKV,
Owner: registry.OwnerSQLQueries,
Cluster: r.MakeClusterSpec(tc.nodes),
Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
runCopyFromCRDB(ctx, t, c, tc.sf, true /*atomic*/)
},
})
r.Add(registry.TestSpec{
Name: fmt.Sprintf("copyfrom/crdb-nonatomic/sf=%d/nodes=%d", tc.sf, tc.nodes),
Owner: registry.OwnerKV,
Owner: registry.OwnerSQLQueries,
Cluster: r.MakeClusterSpec(tc.nodes),
Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
runCopyFromCRDB(ctx, t, c, tc.sf, false /*atomic*/)
},
})
r.Add(registry.TestSpec{
Name: fmt.Sprintf("copyfrom/pg/sf=%d/nodes=%d", tc.sf, tc.nodes),
Owner: registry.OwnerKV,
Owner: registry.OwnerSQLQueries,
Cluster: r.MakeClusterSpec(tc.nodes),
Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
runCopyFromPG(ctx, t, c, tc.sf)
Expand Down
2 changes: 2 additions & 0 deletions pkg/server/autoconfig/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ go_library(
"//pkg/server/autoconfig/autoconfigpb",
"//pkg/settings/cluster",
"//pkg/sql",
"//pkg/sql/catalog/descs",
"//pkg/sql/isql",
"//pkg/sql/sessiondata",
"//pkg/util/encoding",
Expand Down Expand Up @@ -54,6 +55,7 @@ go_test(
"//pkg/testutils/testcluster",
"//pkg/util/leaktest",
"//pkg/util/randutil",
"//pkg/util/syncutil",
"@com_github_stretchr_testify//require",
],
)
Expand Down
7 changes: 6 additions & 1 deletion pkg/server/autoconfig/auto_config_task.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/server/autoconfig/autoconfigpb"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/isql"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
"github.com/cockroachdb/cockroach/pkg/util/log"
Expand Down Expand Up @@ -156,7 +157,11 @@ func execSimpleSQL(

// Now execute all the transactional, potentially not idempotent
// statements.
return execCfg.InternalDB.Txn(ctx, func(ctx context.Context, txn isql.Txn) error {
//
// We use DescsTxn here because the tasks may contain DDL statements
// and we want to ensure that each txn will wait for the leases from
// previous txns.
return execCfg.InternalDB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error {
for _, stmt := range sqlPayload.TransactionalStatements {
log.Infof(ctx, "attempting execution of task statement:\n%s", stmt)
_, err := txn.ExecEx(ctx, "exec-task-statement",
Expand Down
42 changes: 32 additions & 10 deletions pkg/server/autoconfig/auto_config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,15 @@ import (
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/syncutil"
"github.com/stretchr/testify/require"
)

const testEnvID autoconfigpb.EnvironmentID = "my test env"

type testProvider struct {
syncutil.Mutex

t *testing.T
notifyCh chan struct{}
peekWaitCh chan struct{}
Expand All @@ -45,23 +48,25 @@ var testTasks = []testTask{
{task: autoconfigpb.Task{
TaskID: 123,
Description: "test task that creates a system table",
MinVersion: clusterversion.ByKey(clusterversion.V23_1Start),
MinVersion: clusterversion.TestingBinaryVersion,
Payload: &autoconfigpb.Task_SimpleSQL{
SimpleSQL: &autoconfigpb.SimpleSQL{
UsernameProto: username.NodeUserName().EncodeProto(),
NonTransactionalStatements: []string{
"CREATE TABLE IF NOT EXISTS system.foo(x INT)",
// This checks that the non-txn part works properly: SET
// CLUSTER SETTING can only be run outside of explicit txns.
"SET CLUSTER SETTING cluster.organization = 'woo'",
},
TransactionalStatements: []string{
"CREATE TABLE IF NOT EXISTS system.foo(x INT)",
},
},
},
}},
{task: autoconfigpb.Task{
TaskID: 345,
Description: "test task that fails with an error",
MinVersion: clusterversion.ByKey(clusterversion.V23_1Start),
MinVersion: clusterversion.TestingBinaryVersion,
Payload: &autoconfigpb.Task_SimpleSQL{
SimpleSQL: &autoconfigpb.SimpleSQL{
TransactionalStatements: []string{"SELECT invalid"},
Expand All @@ -71,11 +76,11 @@ var testTasks = []testTask{
{task: autoconfigpb.Task{
TaskID: 456,
Description: "test task that creates another system table",
MinVersion: clusterversion.ByKey(clusterversion.V23_1Start),
MinVersion: clusterversion.TestingBinaryVersion,
Payload: &autoconfigpb.Task_SimpleSQL{
SimpleSQL: &autoconfigpb.SimpleSQL{
UsernameProto: username.NodeUserName().EncodeProto(),
NonTransactionalStatements: []string{"CREATE TABLE IF NOT EXISTS system.bar(y INT)"},
UsernameProto: username.NodeUserName().EncodeProto(),
TransactionalStatements: []string{"CREATE TABLE IF NOT EXISTS system.bar(y INT)"},
},
},
}},
Expand All @@ -93,6 +98,9 @@ func (p *testProvider) ActiveEnvironments() []autoconfigpb.EnvironmentID {
func (p *testProvider) Pop(
_ context.Context, envID autoconfigpb.EnvironmentID, taskID autoconfigpb.TaskID,
) error {
p.Lock()
defer p.Unlock()

p.t.Logf("runner reports completed task %d (env %q)", taskID, envID)
for len(p.tasks) > 0 {
if taskID >= p.tasks[0].task.TaskID {
Expand All @@ -105,14 +113,25 @@ func (p *testProvider) Pop(
return nil
}

func (p *testProvider) head() (bool, *testTask) {
p.Lock()
defer p.Unlock()

if len(p.tasks) == 0 {
return false, nil
}
return true, &p.tasks[0]
}

func (p *testProvider) Peek(
ctx context.Context, envID autoconfigpb.EnvironmentID,
) (autoconfigpb.Task, error) {
p.t.Logf("runner peeking (env %q)", envID)
if len(p.tasks) == 0 {
hasTask, tt := p.head()
if !hasTask {
return autoconfigpb.Task{}, acprovider.ErrNoMoreTasks
}
if !p.tasks[0].seen {
if !tt.seen {
// seen ensures that the runner job won't have to wait a second
// time when peeking the task.
select {
Expand All @@ -121,8 +140,11 @@ func (p *testProvider) Peek(
case <-p.peekWaitCh:
}
}
p.tasks[0].seen = true
return p.tasks[0].task, nil

p.Lock()
defer p.Unlock()
tt.seen = true
return tt.task, nil
}

func TestAutoConfig(t *testing.T) {
Expand Down
56 changes: 48 additions & 8 deletions pkg/sql/opt/ordering/ordering.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func BuildProvided(expr memo.RelExpr, required *props.OrderingChoice) opt.Orderi
return nil
}
provided := funcMap[expr.Op()].buildProvidedOrdering(expr, required)
provided = finalizeProvided(provided, required, expr.Relational().OutputCols)

if buildutil.CrdbTestBuild {
checkProvided(expr, required, provided)
Expand Down Expand Up @@ -423,6 +424,53 @@ func trimProvided(
return provided[:provIdx]
}

// finalizeProvided ensures that the provided ordering satisfies the following
// properties:
// 1. The provided ordering can be proven to satisfy the required ordering
// without the use of additional (e.g. functional dependency) information.
// 2. The provided ordering is simplified, such that it does not contain any
// columns from the required ordering optional set.
// 3. The provided ordering only refers to output columns for the operator.
//
// This step is necessary because it is possible for child operators to have
// different functional dependency information than their parents as well as
// different output columns. We have to protect against the case where a parent
// operator cannot prove that its child's provided ordering satisfies its
// required ordering.
func finalizeProvided(
provided opt.Ordering, required *props.OrderingChoice, outCols opt.ColSet,
) (newProvided opt.Ordering) {
// First check if the given provided is already suitable.
providedCols := provided.ColSet()
if len(provided) == len(required.Columns) && providedCols.SubsetOf(outCols) {
needsRemap := false
for i := range provided {
choice, ordCol := required.Columns[i], provided[i]
if !choice.Group.Contains(ordCol.ID()) || choice.Descending != ordCol.Descending() {
needsRemap = true
break
}
}
if !needsRemap {
return provided
}
}
newProvided = make(opt.Ordering, len(required.Columns))
for i, choice := range required.Columns {
group := choice.Group.Intersection(outCols)
if group.Intersects(providedCols) {
// Prefer using columns from the provided ordering if possible.
group.IntersectionWith(providedCols)
}
col, ok := group.Next(0)
if !ok {
panic(errors.AssertionFailedf("no output column equivalent to %d", redact.Safe(col)))
}
newProvided[i] = opt.MakeOrderingColumn(col, choice.Descending)
}
return newProvided
}

// checkRequired runs sanity checks on the ordering required of an operator.
func checkRequired(expr memo.RelExpr, required *props.OrderingChoice) {
rel := expr.Relational()
Expand Down Expand Up @@ -472,12 +520,4 @@ func checkProvided(expr memo.RelExpr, required *props.OrderingChoice, provided o
))
}
}

// The provided ordering should not have unnecessary columns.
fds := &expr.Relational().FuncDeps
if trimmed := trimProvided(provided, required, fds); len(trimmed) != len(provided) {
panic(errors.AssertionFailedf(
"provided %s can be trimmed to %s (FDs: %s)", redact.Safe(provided), redact.Safe(trimmed), redact.Safe(fds),
))
}
}
Loading

0 comments on commit 668ec8d

Please sign in to comment.