Skip to content

Conversation

@michae2
Copy link
Collaborator

@michae2 michae2 commented Sep 26, 2024

Backport 2/2 commits from #130375.

/cc @cockroachdb/release


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


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


Release justification: fix for flaky logictest.

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
@michae2 michae2 requested review from a team and rafiss September 26, 2024 17:38
@blathers-crl
Copy link

blathers-crl bot commented Sep 26, 2024

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Backports should only be created for serious
    issues
    or test-only changes.
  • Backports should not break backwards-compatibility.
  • Backports should change as little code as possible.
  • Backports should not change on-disk formats or node communication protocols.
  • Backports should not add new functionality (except as defined
    here).
  • Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
  • All backports must be reviewed by the owning areas TL. For more information as to how that review should be conducted, please consult the backport
    policy
    .
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
  • Your backport must be accompanied by a post to the appropriate Slack
    channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Sep 26, 2024
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)

@michae2 michae2 merged commit b293f4e into cockroachdb:release-24.2 Oct 1, 2024
@michae2 michae2 deleted the backport24.2-130375 branch October 1, 2024 17:23
@crl-codesys-jira crl-codesys-jira added the T-sql-queries SQL Queries Team label Aug 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport Label PR's that are backports to older release branches T-sql-queries SQL Queries Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants