From 5e3338374af031b5b2d054bfc782f66d0b8b79ff Mon Sep 17 00:00:00 2001 From: Rebecca Taft Date: Fri, 2 Jun 2023 18:09:37 -0500 Subject: [PATCH] sql: make transaction_rows_read_err prevent large scans Prior to this commit, setting transaction_rows_read_err to a non-zero value would cause a transaction to fail as soon as a statement caused the total number of rows read to exceed transaction_rows_read_err. However, it was possible that each statement could read many more than transaction_rows_read_err rows. This commit adds logic so that a single scan never reads more than transaction_rows_read_err+1 rows if transaction_rows_read_err is set. Informs #70473 Release note (performance improvement): If transaction_rows_read_err is set to a non-zero value, we now ensure that any single scan never reads more than transaction_rows_read_err+1 rows. This prevents transactions that would error due to the transaction_rows_read_err setting from causing a large performance overhead due to large scans. For some queries in rare cases this change may end up disabling cross-range parallelism of the scan operation which can result in increase of query latency. --- .../tests/3node-tenant/generated_test.go | 7 + .../logictest/testdata/logic_test/guardrails | 74 ++++++++++ .../tests/fakedist-disk/generated_test.go | 7 + .../tests/fakedist-vec-off/generated_test.go | 7 + .../tests/fakedist/generated_test.go | 7 + .../generated_test.go | 7 + .../tests/local-vec-off/generated_test.go | 7 + .../logictest/tests/local/generated_test.go | 7 + pkg/sql/opt/exec/execbuilder/relational.go | 18 ++- .../opt/exec/execbuilder/testdata/guardrails | 133 ++++++++++++++++++ .../execbuilder/tests/local/generated_test.go | 7 + pkg/sql/opt/memo/memo.go | 3 + pkg/sql/opt/memo/memo_test.go | 6 + 13 files changed, 286 insertions(+), 4 deletions(-) create mode 100644 pkg/sql/logictest/testdata/logic_test/guardrails create mode 100644 pkg/sql/opt/exec/execbuilder/testdata/guardrails diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index bb28cfe3b94a..504631239ccf 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -904,6 +904,13 @@ func TestTenantLogic_grant_schema( runLogicTest(t, "grant_schema") } +func TestTenantLogic_guardrails( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "guardrails") +} + func TestTenantLogic_hash_join( t *testing.T, ) { diff --git a/pkg/sql/logictest/testdata/logic_test/guardrails b/pkg/sql/logictest/testdata/logic_test/guardrails new file mode 100644 index 000000000000..b80720435648 --- /dev/null +++ b/pkg/sql/logictest/testdata/logic_test/guardrails @@ -0,0 +1,74 @@ +statement ok +CREATE TABLE guardrails (i INT PRIMARY KEY); +INSERT INTO guardrails SELECT generate_series(1, 100) + +# When the transaction_rows_read_err guardrail is set to 1, we apply a limit +# of 2 in all cases except for when we know at most 2 rows are returned. +statement ok +SET transaction_rows_read_err = 1 + +query error txn has read 2 rows, which is above the limit +SELECT * FROM guardrails + +query error txn has read 2 rows, which is above the limit +SELECT * FROM guardrails LIMIT 50 + +statement ok +SELECT * FROM guardrails WHERE i = 1 + +statement error txn has read 2 rows, which is above the limit +SELECT * FROM guardrails WHERE i IN (1, 2) + +query error txn has read 2 rows, which is above the limit +SELECT * FROM guardrails WHERE i > 0 AND i <= 10 + +# When the transaction_rows_read_err guardrail is set to 50, we only apply a +# limit if it's possible that more than 51 rows may be returned. +statement ok +SET transaction_rows_read_err = 50 + +query error txn has read 51 rows, which is above the limit +SELECT * FROM guardrails + +statement ok +SELECT * FROM guardrails LIMIT 50 + +statement ok +SELECT * FROM guardrails WHERE i = 1 + +statement ok +SELECT * FROM guardrails WHERE i > 0 AND i <= 10 + +statement ok +SET transaction_rows_read_err = 150 + +statement ok +CREATE TABLE guardrails2 (i INT PRIMARY KEY); +INSERT INTO guardrails2 SELECT generate_series(1, 150) + +statement ok +BEGIN + +# A full scan shouldn't error if it only scans transaction_rows_read_err rows. +statement ok +SELECT * FROM guardrails2 + +statement ok +COMMIT + +statement ok +BEGIN + +statement ok +SELECT * FROM guardrails + +# The whole transaction has now read more than transaction_rows_read_err rows, +# so error. +query error txn has read 250 rows, which is above the limit +SELECT * FROM guardrails2 + +statement ok +ROLLBACK + +statement ok +RESET transaction_rows_read_err diff --git a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go index fc5e9bbb8669..6141cb403af6 100644 --- a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go @@ -870,6 +870,13 @@ func TestLogic_grant_schema( runLogicTest(t, "grant_schema") } +func TestLogic_guardrails( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "guardrails") +} + func TestLogic_hash_join( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go index c59f99d62c2b..93a9bf2c07bd 100644 --- a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go @@ -870,6 +870,13 @@ func TestLogic_grant_schema( runLogicTest(t, "grant_schema") } +func TestLogic_guardrails( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "guardrails") +} + func TestLogic_hash_join( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist/generated_test.go b/pkg/sql/logictest/tests/fakedist/generated_test.go index 74c9d0b4e28f..8386afa4176d 100644 --- a/pkg/sql/logictest/tests/fakedist/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist/generated_test.go @@ -877,6 +877,13 @@ func TestLogic_grant_schema( runLogicTest(t, "grant_schema") } +func TestLogic_guardrails( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "guardrails") +} + func TestLogic_hash_join( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go index d69f2b60c254..86dbb1fd057b 100644 --- a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go +++ b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go @@ -856,6 +856,13 @@ func TestLogic_grant_schema( runLogicTest(t, "grant_schema") } +func TestLogic_guardrails( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "guardrails") +} + func TestLogic_hash_join( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-vec-off/generated_test.go b/pkg/sql/logictest/tests/local-vec-off/generated_test.go index 4b2a85a83301..9999438f6b91 100644 --- a/pkg/sql/logictest/tests/local-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/local-vec-off/generated_test.go @@ -877,6 +877,13 @@ func TestLogic_grant_schema( runLogicTest(t, "grant_schema") } +func TestLogic_guardrails( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "guardrails") +} + func TestLogic_hash_join( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local/generated_test.go b/pkg/sql/logictest/tests/local/generated_test.go index b5d3b29463b6..914b8b6ff93d 100644 --- a/pkg/sql/logictest/tests/local/generated_test.go +++ b/pkg/sql/logictest/tests/local/generated_test.go @@ -933,6 +933,13 @@ func TestLogic_grant_type( runLogicTest(t, "grant_type") } +func TestLogic_guardrails( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "guardrails") +} + func TestLogic_hash_join( t *testing.T, ) { diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index 745a25f506c4..102f808d0f3a 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -639,6 +639,17 @@ func (b *Builder) scanParams( softLimit := reqProps.LimitHintInt64() hardLimit := scan.HardLimit.RowCount() + maxResults, maxResultsOk := b.indexConstraintMaxResults(scan, relProps) + + // If the txn_rows_read_err guardrail is set, make sure that we never read + // more than txn_rows_read_err+1 rows on any single scan. Adding a hard limit + // of txn_rows_read_err+1 ensures that the results will still be correct since + // the conn_executor will return an error if the limit is actually reached. + if txnRowsReadErr := b.evalCtx.SessionData().TxnRowsReadErr; txnRowsReadErr > 0 && + (hardLimit == 0 || hardLimit > txnRowsReadErr+1) && + (!maxResultsOk || maxResults > uint64(txnRowsReadErr+1)) { + hardLimit = txnRowsReadErr + 1 + } // If this is a bounded staleness query, check that it touches at most one // range. @@ -653,8 +664,7 @@ func (b *Builder) scanParams( // multiple ranges if the first range is empty. valid = false } else { - maxResults, ok := b.indexConstraintMaxResults(scan, relProps) - valid = ok && maxResults == 1 + valid = maxResultsOk && maxResults == 1 } if !valid { return exec.ScanParams{}, opt.ColMap{}, unimplemented.NewWithIssuef(67562, @@ -666,8 +676,8 @@ func (b *Builder) scanParams( parallelize := false if hardLimit == 0 && softLimit == 0 { - maxResults, ok := b.indexConstraintMaxResults(scan, relProps) - if ok && maxResults < getParallelScanResultThreshold(b.evalCtx.TestingKnobs.ForceProductionValues) { + if maxResultsOk && + maxResults < getParallelScanResultThreshold(b.evalCtx.TestingKnobs.ForceProductionValues) { // Don't set the flag when we have a single span which returns a single // row: it does nothing in this case except litter EXPLAINs. // There are still cases where the flag doesn't do anything when the spans diff --git a/pkg/sql/opt/exec/execbuilder/testdata/guardrails b/pkg/sql/opt/exec/execbuilder/testdata/guardrails new file mode 100644 index 000000000000..4a0162f6ff5b --- /dev/null +++ b/pkg/sql/opt/exec/execbuilder/testdata/guardrails @@ -0,0 +1,133 @@ +# LogicTest: local + +statement ok +CREATE TABLE guardrails (i INT PRIMARY KEY) + +# When the transaction_rows_read_err guardrail is set to 1, we apply a limit +# of 2 in all cases except for when we know at most 2 rows are returned. +statement ok +SET transaction_rows_read_err = 1 + +query T +EXPLAIN (VERBOSE) SELECT * FROM guardrails +---- +distribution: local +vectorized: true +· +• scan + columns: (i) + estimated row count: 1,000 (missing stats) + table: guardrails@guardrails_pkey + spans: LIMITED SCAN + limit: 2 + +query T +EXPLAIN (VERBOSE) SELECT * FROM guardrails LIMIT 50 +---- +distribution: local +vectorized: true +· +• scan + columns: (i) + estimated row count: 50 (missing stats) + table: guardrails@guardrails_pkey + spans: LIMITED SCAN + limit: 2 + +query T +EXPLAIN (VERBOSE) SELECT * FROM guardrails WHERE i = 1 +---- +distribution: local +vectorized: true +· +• scan + columns: (i) + estimated row count: 1 (missing stats) + table: guardrails@guardrails_pkey + spans: /1/0 + +query T +EXPLAIN (VERBOSE) SELECT * FROM guardrails WHERE i IN (1, 2) +---- +distribution: local +vectorized: true +· +• scan + columns: (i) + estimated row count: 2 (missing stats) + table: guardrails@guardrails_pkey + spans: /1-/3 + parallel + + +query T +EXPLAIN (VERBOSE) SELECT * FROM guardrails WHERE i > 0 AND i <= 10 +---- +distribution: local +vectorized: true +· +• scan + columns: (i) + estimated row count: 10 (missing stats) + table: guardrails@guardrails_pkey + spans: /1-/11 + limit: 2 + +# When the transaction_rows_read_err guardrail is set to 50, we only apply a +# limit if it's possible that more than 51 rows may be returned. +statement ok +SET transaction_rows_read_err = 50 + +query T +EXPLAIN (VERBOSE) SELECT * FROM guardrails +---- +distribution: local +vectorized: true +· +• scan + columns: (i) + estimated row count: 1,000 (missing stats) + table: guardrails@guardrails_pkey + spans: LIMITED SCAN + limit: 51 + +query T +EXPLAIN (VERBOSE) SELECT * FROM guardrails LIMIT 50 +---- +distribution: local +vectorized: true +· +• scan + columns: (i) + estimated row count: 50 (missing stats) + table: guardrails@guardrails_pkey + spans: LIMITED SCAN + limit: 50 + +query T +EXPLAIN (VERBOSE) SELECT * FROM guardrails WHERE i = 1 +---- +distribution: local +vectorized: true +· +• scan + columns: (i) + estimated row count: 1 (missing stats) + table: guardrails@guardrails_pkey + spans: /1/0 + +query T +EXPLAIN (VERBOSE) SELECT * FROM guardrails WHERE i > 0 AND i <= 10 +---- +distribution: local +vectorized: true +· +• scan + columns: (i) + estimated row count: 10 (missing stats) + table: guardrails@guardrails_pkey + spans: /1-/11 + parallel + +statement ok +RESET transaction_rows_read_err diff --git a/pkg/sql/opt/exec/execbuilder/tests/local/generated_test.go b/pkg/sql/opt/exec/execbuilder/tests/local/generated_test.go index 4f7878c1355c..9af0e0c76217 100644 --- a/pkg/sql/opt/exec/execbuilder/tests/local/generated_test.go +++ b/pkg/sql/opt/exec/execbuilder/tests/local/generated_test.go @@ -228,6 +228,13 @@ func TestExecBuild_geospatial( runExecBuildLogicTest(t, "geospatial") } +func TestExecBuild_guardrails( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runExecBuildLogicTest(t, "guardrails") +} + func TestExecBuild_hash_sharded_index( t *testing.T, ) { diff --git a/pkg/sql/opt/memo/memo.go b/pkg/sql/opt/memo/memo.go index 4c82817df7a4..146b647675ce 100644 --- a/pkg/sql/opt/memo/memo.go +++ b/pkg/sql/opt/memo/memo.go @@ -150,6 +150,7 @@ type Memo struct { propagateInputOrdering bool disallowFullTableScans bool largeFullScanRows float64 + txnRowsReadErr int64 nullOrderedLast bool costScansWithDefaultColSize bool allowUnconstrainedNonCoveringIndexScan bool @@ -209,6 +210,7 @@ func (m *Memo) Init(evalCtx *eval.Context) { propagateInputOrdering: evalCtx.SessionData().PropagateInputOrdering, disallowFullTableScans: evalCtx.SessionData().DisallowFullTableScans, largeFullScanRows: evalCtx.SessionData().LargeFullScanRows, + txnRowsReadErr: evalCtx.SessionData().TxnRowsReadErr, nullOrderedLast: evalCtx.SessionData().NullOrderedLast, costScansWithDefaultColSize: evalCtx.SessionData().CostScansWithDefaultColSize, allowUnconstrainedNonCoveringIndexScan: evalCtx.SessionData().UnconstrainedNonCoveringIndexScanEnabled, @@ -352,6 +354,7 @@ func (m *Memo) IsStale( m.propagateInputOrdering != evalCtx.SessionData().PropagateInputOrdering || m.disallowFullTableScans != evalCtx.SessionData().DisallowFullTableScans || m.largeFullScanRows != evalCtx.SessionData().LargeFullScanRows || + m.txnRowsReadErr != evalCtx.SessionData().TxnRowsReadErr || m.nullOrderedLast != evalCtx.SessionData().NullOrderedLast || m.costScansWithDefaultColSize != evalCtx.SessionData().CostScansWithDefaultColSize || m.allowUnconstrainedNonCoveringIndexScan != evalCtx.SessionData().UnconstrainedNonCoveringIndexScanEnabled || diff --git a/pkg/sql/opt/memo/memo_test.go b/pkg/sql/opt/memo/memo_test.go index ffa8ea0d79df..66cbb367682f 100644 --- a/pkg/sql/opt/memo/memo_test.go +++ b/pkg/sql/opt/memo/memo_test.go @@ -268,6 +268,12 @@ func TestMemoIsStale(t *testing.T) { evalCtx.SessionData().LargeFullScanRows = 0 notStale() + // Stale txn rows read error. + evalCtx.SessionData().TxnRowsReadErr = 1000 + stale() + evalCtx.SessionData().TxnRowsReadErr = 0 + notStale() + // Stale null ordered last. evalCtx.SessionData().NullOrderedLast = true stale()