From 68e2f4b45bdb63ce02a71def5986f63405d799bb Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Mon, 9 Sep 2024 16:13:11 -0700 Subject: [PATCH 1/2] logictest: set session variables in connection URL To execute SQL statements, logic tests use a `sql.DB` connection pool managed by database/sql and backed by the lib/pq driver. This connection pool can open new connections as necessary, but typically does not because most `query` and `statement` commands block until the SQL statement finishes, allowing the connection pool to continue reusing the same single connection. Occasionally, however, the connection pool does open a new connection, for example whenever `statement async` and `query async` commands return before the SQL statement has finished. Another example is when lib/pq returns `driver.ErrBadConn`, telling the connection pool to retry a query using a new connection. These new connections are opened internally by the connection pool, without going through `getOrOpenClient`, meaning they won't have any of the session variables from `getOrOpenClient` set. This test failure was one such case, where after a connection-pool-level retry the `default_transaction_isolation` of an UPDATE statement was the default of SERIALIZABLE. To ensure these new connections have the correct session variables set, this commit adds them to the connection URL passed to lib/pq. This will set them on every new connection, even those opened internally by the connection pool. Fixes: #129921 Release note: None --- pkg/sql/logictest/logic.go | 31 +++++++++++-------- .../logictest/testdata/logic_test/pg_catalog | 4 +-- pkg/sql/logictest/testdata/logic_test/txn | 30 ++++++++++++++++-- 3 files changed, 47 insertions(+), 18 deletions(-) diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go index 7476a44d6d05..9ade431f3589 100644 --- a/pkg/sql/logictest/logic.go +++ b/pkg/sql/logictest/logic.go @@ -1253,26 +1253,31 @@ func (t *logicTest) getOrOpenClient(user string, nodeIdx int, newSession bool) * } pgURL.Path = "test" - db := t.openDB(pgURL) - - // The default value for extra_float_digits assumed by tests is - // 1. However, lib/pq by default configures this to 2 during - // connection initialization, so we need to set it back to 1 before - // we run anything. - if _, err := db.Exec("SET extra_float_digits = 1"); err != nil { + // Set some session variables to non-default values in every connection. We do + // this via PG URL options rather than SET SQL statements because we need + // lib/pq to set these session variables in every new connection created for + // the database/sql connection pool. + opts, err := url.ParseQuery(pgURL.RawQuery) + if err != nil { t.Fatal(err) } + // The default value for extra_float_digits assumed by tests is 1. However, + // lib/pq by default configures this to 2 during connection initialization, so + // we need to set it back to 1 before we run anything. + opts.Add("extra_float_digits", "1") // The default setting for index_recommendations_enabled is true. We do not // want to display index recommendations in logic tests, so we disable them // here. - if _, err := db.Exec("SET index_recommendations_enabled = false"); err != nil { - t.Fatal(err) - } + opts.Add("index_recommendations_enabled", "false") + + // Set default transaction isolation if it is not serializable. if t.cfg.EnableDefaultReadCommitted { - if _, err := db.Exec("SET default_transaction_isolation = 'READ COMMITTED'"); err != nil { - t.Fatal(err) - } + opts.Add("default_transaction_isolation", "READ COMMITTED") } + pgURL.RawQuery = opts.Encode() + + db := t.openDB(pgURL) + if t.clients == nil { t.clients = make(map[string]map[int]*gosql.DB) } diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index 30f6a915839c..ef7d011e8d48 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -3056,13 +3056,13 @@ experimental_enable_implicit_column_partitioning off N experimental_enable_temp_tables off NULL user NULL off off experimental_enable_unique_without_index_constraints on NULL user NULL off off experimental_hash_group_join_enabled off NULL user NULL off off -extra_float_digits 1 NULL user NULL 1 2 +extra_float_digits 1 NULL user NULL 1 1 force_savepoint_restart off NULL user NULL off off foreign_key_cascades_limit 10000 NULL user NULL 10000 10000 idle_in_transaction_session_timeout 0 NULL user NULL 0s 0s idle_session_timeout 0 NULL user NULL 0s 0s index_join_streamer_batch_size 8.0 MiB NULL user NULL 8.0 MiB 8.0 MiB -index_recommendations_enabled off NULL user NULL on on +index_recommendations_enabled off NULL user NULL on false inject_retry_errors_enabled off NULL user NULL off off inject_retry_errors_on_commit_enabled off NULL user NULL off off integer_datetimes on NULL user NULL on on diff --git a/pkg/sql/logictest/testdata/logic_test/txn b/pkg/sql/logictest/testdata/logic_test/txn index 0bfd64dd09f6..8f94e09650c2 100644 --- a/pkg/sql/logictest/testdata/logic_test/txn +++ b/pkg/sql/logictest/testdata/logic_test/txn @@ -1031,11 +1031,18 @@ COMMIT statement ok RESET DEFAULT_TRANSACTION_ISOLATION +skipif config local-read-committed query T SHOW DEFAULT_TRANSACTION_ISOLATION ---- serializable +onlyif config local-read-committed +query T +SHOW DEFAULT_TRANSACTION_ISOLATION +---- +read committed + # SHOW TRANSACTION STATUS query T @@ -1099,9 +1106,14 @@ COMMIT statement ok BEGIN TRANSACTION; SAVEPOINT cockroach_restart; SELECT 1 +skipif config local-read-committed query error pgcode 40001 restart transaction: crdb_internal.force_retry\(\): TransactionRetryWithProtoRefreshError: forced by crdb_internal.force_retry\(\).*\nHINT:.*transaction-retry-error-reference.html SELECT crdb_internal.force_retry('1h':::INTERVAL) +onlyif config local-read-committed +query error pgcode 40001 pq: restart transaction: read committed retry limit exceeded; set by max_retries_for_read_committed=10: crdb_internal.force_retry\(\): TransactionRetryWithProtoRefreshError: forced by crdb_internal.force_retry\(\) +SELECT crdb_internal.force_retry('1h':::INTERVAL) + query T SHOW TRANSACTION STATUS ---- @@ -1236,9 +1248,14 @@ BEGIN TRANSACTION; SAVEPOINT cockroach_restart; SELECT 1; +skipif config local-read-committed query error pgcode 40001 restart transaction: crdb_internal.force_retry\(\): TransactionRetryWithProtoRefreshError: forced by crdb_internal.force_retry\(\) SELECT crdb_internal.force_retry('1h':::INTERVAL) +onlyif config local-read-committed +query error pgcode 40001 pq: restart transaction: read committed retry limit exceeded; set by max_retries_for_read_committed=10: crdb_internal.force_retry\(\): TransactionRetryWithProtoRefreshError: forced by crdb_internal.force_retry\(\) +SELECT crdb_internal.force_retry('1h':::INTERVAL) + statement ok ROLLBACK TO SAVEPOINT COCKROACH_RESTART; @@ -1497,9 +1514,14 @@ BEGIN ISOLATION LEVEL SERIALIZABLE, ISOLATION LEVEL SERIALIZABLE statement ok BEGIN; SELECT 1 +skipif config local-read-committed query error pgcode 40001 restart transaction: crdb_internal.force_retry\(\): TransactionRetryWithProtoRefreshError: forced by crdb_internal.force_retry\(\) SELECT crdb_internal.force_retry('1h':::INTERVAL) +onlyif config local-read-committed +query error pgcode 40001 pq: restart transaction: read committed retry limit exceeded; set by max_retries_for_read_committed=10: crdb_internal.force_retry\(\): TransactionRetryWithProtoRefreshError: forced by crdb_internal.force_retry\(\) +SELECT crdb_internal.force_retry('1h':::INTERVAL) + statement ok ROLLBACK @@ -1587,7 +1609,9 @@ SET SESSION CHARACTERISTICS AS TRANSACTION DEFERRABLE # Test retry rewinds correctly. statement ok -SET intervalstyle = 'postgres'; +SET intervalstyle = 'postgres' + +statement ok CREATE TABLE rewind_session_test (s string primary key); statement ok @@ -1610,10 +1634,10 @@ SHOW intervalstyle iso_8601 statement ok -TRUNCATE rewind_session_test; +TRUNCATE rewind_session_test statement ok -SET intervalstyle = 'postgres'; +SET intervalstyle = 'postgres' statement ok BEGIN; From a576cfc042476946b10f4b078780e0604fbc6d24 Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Mon, 9 Sep 2024 16:15:57 -0700 Subject: [PATCH 2/2] logictest: fix another flake in select_for_share While stressing select_for_share for #129921, I noticed another rare flake, where we sometimes have an unexpected shared lock appear in `crdb_internal.cluster_locks` after a single SELECT FOR SHARE statement. I think what's happening here is that a retry of the SELECT FOR SHARE statement interacts with the shared lock from the previous execution, causing an entry to appear in `crdb_internal.cluster_locks`. (We don't always immediately clean up locks after a transaction finishes, which is expected.) I've simply removed the query. I don't think it's necessary to prove correctness, as long as the following SELECT FOR UPDATE SKIP LOCKED returns the expected row. Informs: #129921 Release note: None --- pkg/sql/logictest/testdata/logic_test/select_for_share | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/select_for_share b/pkg/sql/logictest/testdata/logic_test/select_for_share index 9d0889242bef..0ef465494dc1 100644 --- a/pkg/sql/logictest/testdata/logic_test/select_for_share +++ b/pkg/sql/logictest/testdata/logic_test/select_for_share @@ -207,11 +207,6 @@ SELECT * FROM t WHERE a = 1 FOR SHARE; user root -query TTTTTTTBB colnames,retry,rowsort -SELECT database_name, schema_name, table_name, lock_key_pretty, lock_strength, durability, isolation_level, granted, contended FROM crdb_internal.cluster_locks ----- -database_name schema_name table_name lock_key_pretty lock_strength durability isolation_level granted contended - # Locks: # 1: Shared # 2: None