logictest: fix flakes in select_for_share #130375
Conversation
michae2
added
backport-23.2.x
|
This change is |
rafiss
left a comment
rafiss
left a comment
There was a problem hiding this comment.
commit #1 lgtm! nice find and fix!
i'm fine with commit #2 also, but i'm less familiar with the expected locking behavior.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)
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: cockroachdb#129921 Release note: None
While stressing select_for_share for cockroachdb#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: cockroachdb#129921 Release note: None
|
TFTR! bors r=rafiss |
|
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #129921: branch-release-23.2, branch-release-24.1, branch-release-24.2. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from c340f4f to blathers/backport-release-23.2-130375: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2.x failed. See errors above. error creating merge commit from c340f4f to blathers/backport-release-24.1-130375: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.1.x failed. See errors above. error creating merge commit from c340f4f to blathers/backport-release-24.2-130375: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
3 participants
logictest: set session variables in connection URL
To execute SQL statements, logic tests use a
sql.DBconnection poolmanaged 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
queryandstatementcommands block until the SQLstatement 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 asyncandquery asynccommands returnbefore the SQL statement has finished. Another example is when lib/pq
returns
driver.ErrBadConn, telling the connection pool to retry aquery using a new connection.
These new connections are opened internally by the connection pool,
without going through
getOrOpenClient, meaning they won't have any ofthe session variables from
getOrOpenClientset. This test failure wasone such case, where after a connection-pool-level retry the
default_transaction_isolationof an UPDATE statement was the defaultof 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
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_locksafter 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'talways 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