go.mod: bump pgx to v5.9.2 for OAuthTokenProvider support#171289
go.mod: bump pgx to v5.9.2 for OAuthTokenProvider support#171289darshanjain10 wants to merge 1 commit into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
|
|
This change is |
darshanjain10
force-pushed
the
bump-pgx-v5.9.2
branch
from
a0d0d05 to
f44db51
Compare
cockroach-teamcity
added
the
X-perf-gain
darshanjain10
force-pushed
the
bump-pgx-v5.9.2
branch
from
f44db51 to
8774479
Compare
darshanjain10
changed the title
darshanjain10
force-pushed
the
bump-pgx-v5.9.2
branch
from
8774479 to
f60a3a9
Compare
darshanjain10
force-pushed
the
bump-pgx-v5.9.2
branch
from
f60a3a9 to
be62514
Compare
darshanjain10
requested review from
cpj2195 and
williamchoe3
and removed request for
a team
darshanjain10
force-pushed
the
bump-pgx-v5.9.2
branch
from
be62514 to
ee62389
Compare
Closes #171149 Release note: None |
sanchit-CRL
left a comment
sanchit-CRL
left a comment
There was a problem hiding this comment.
PR title (...for OAuthTokenProvider support) is more informative than the commit headline (go.mod: bump pgx to v5.9.2). Aligning the commit headline.
| cancelRequest := proxyCancelRequest{ | ||
| ProxyIP: net.IP{}, | ||
| SecretKey: conn.PgConn().SecretKey(), | ||
| SecretKey: binary.BigEndian.Uint32(conn.PgConn().SecretKey()), |
There was a problem hiding this comment.
Safe today because CRDB only advertises PG protocol v3.0 (so the BackendKeyData secret is always exactly 4 bytes), but under v3.2 the cancel key is variable length and BigEndian.Uint32 would silently truncate to the first 4 bytes no panic, just a wrong key. Worth either a one-time require.Len(t, key, 4) guard or a small secretKeyAsUint32(t, key) helper to make the v3.0 assumption explicit and load-bearing.
It would be better to get an eye from SQl foundation on this as well
There was a problem hiding this comment.
+1 to assert the size of the key.
I looked at the other changes in here from a SQL Foundations perspective and it all looks good.
darshanjain10
force-pushed
the
bump-pgx-v5.9.2
branch
from
ee62389 to
2cf098d
Compare
|
Detected infrastructure failure (matched: self-hosted runner lost communication with the server). Automatically rerunning failed jobs. (run link) |
spilchen
left a comment
spilchen
left a comment
There was a problem hiding this comment.
@spilchen reviewed 6 files and all commit messages, and made 2 comments.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on cpj2195, sanchit-CRL, and williamchoe3).
| cancelRequest := proxyCancelRequest{ | ||
| ProxyIP: net.IP{}, | ||
| SecretKey: conn.PgConn().SecretKey(), | ||
| SecretKey: binary.BigEndian.Uint32(conn.PgConn().SecretKey()), |
There was a problem hiding this comment.
+1 to assert the size of the key.
I looked at the other changes in here from a SQL Foundations perspective and it all looks good.
Bumps github.com/jackc/pgx/v5 from v5.7.2 to v5.9.2. The primary motivation is that v5.9.2 adds OAuthTokenProvider to pgconn.Config, enabling native OAUTHBEARER SASL client support. Four compatibility fixes are required: 1. pgconn.PgConn.SecretKey() now returns []byte instead of uint32, supporting the new PostgreSQL protocol v3.2 variable-length cancel key. The sqlproxy cancel tests recover the original uint32 via a new pgConnSecretKeyUint32 helper, which asserts the key is exactly 4 bytes. This makes the protocol v3.0 assumption explicit: CockroachDB only speaks v3.0 today so the key is always 4 bytes, and the assertion will catch any future drift rather than silently truncating. 2. pgconn now returns a "conn closed" error from its internal state machine when operations are attempted on an already-closed connection, rather than propagating the OS-level "connection reset by peer" or "unexpected EOF". Two regex assertions in sqlproxy tests are updated to accept all three forms. 3. pgxpool.Config.BeforeAcquire is deprecated in v5.9.2 in favor of PrepareConn, which has the same semantics but also returns an error. The workload pool setup is migrated accordingly. 4. pgx v5.9.2 enforces the PostgreSQL pipeline protocol more strictly: ReadyForQuery must be sent after Sync, not after each Execute. TestPipelineMetric's fake server was sending ReadyForQuery after each Execute (via finishQuery(execPortal)), which worked with v5.7.2's lenient pipeline reader but fails with v5.9.2. Two new test helpers (expectExecPortalPipelined, expectSyncAndSendReadyForQuery) correctly simulate the pipeline response sequence. Closes #171149 Closes #169280 Informs: CRDB-64341 Epic: CRDB-60766 Release note: None
darshanjain10
force-pushed
the
bump-pgx-v5.9.2
branch
from
2cf098d to
7c224c8
Compare
4 participants
Bumps github.com/jackc/pgx/v5 from v5.7.2 to v5.9.2, which adds OAuthTokenProvider to pgconn.Config, enabling native OAUTHBEARER SASL client support needed for subsequent work on the pgwire OAuth flow.
DEPS.bzl updated to use the properly mirrored format after running
./dev generate bazel --mirror.
Four compatibility fixes are required:
pgconn.PgConn.SecretKey() now returns []byte instead of uint32, supporting the new PostgreSQL protocol v3.2 variable-length cancel key. The sqlproxy cancel tests recover the original uint32 via a new pgConnSecretKeyUint32 helper, which asserts the key is exactly 4 bytes. This makes the protocol v3.0 assumption explicit: CockroachDB only speaks v3.0 today so the key is always 4 bytes, and the assertion will catch any future drift rather than silently truncating.
pgconn now returns a "conn closed" error from its internal state machine when operations are attempted on an already-closed connection, rather than propagating the OS-level "connection reset by peer" or "unexpected EOF". Two regex assertions in sqlproxy tests are updated to accept all three forms.
pgxpool.Config.BeforeAcquire is deprecated in v5.9.2 in favor of PrepareConn, which has the same semantics but also returns an error. The workload pool setup is migrated accordingly.
pgx v5.9.2 enforces the PostgreSQL pipeline protocol more strictly: ReadyForQuery must be sent after Sync, not after each Execute. TestPipelineMetric's fake server was sending ReadyForQuery after each Execute (via finishQuery(execPortal)), which worked with v5.7.2's lenient pipeline reader but fails with v5.9.2. Two new test helpers (expectExecPortalPipelined, expectSyncAndSendReadyForQuery) correctly simulate the pipeline response sequence.
Closes #171149
Closes #169280
Informs: CRDB-64341
Epic: CRDB-60766
Release note: None