Skip to content

driver-sql: a redshift datasource gets the pg wire hook but no connect-timeout bound — DIALECT_CONNECT_TIMEOUT has no entry for it, so the 10s dialect bound silently degrades to the 15s pool backstop #11784

Description

@huangyiirene

Summary

SqlDriver answers three separate questions about a knex client name, from
three separate tables in packages/drivers/driver-sql/src/sql-driver.ts:

questiontable
whose SQL do I emit?the dialect-identity sets behind isPostgres / isSqlite / isMysql
which npm package parses the wire format?POSTGRES_WIRE_CLIENTS
how do I spell "bound ONE connection attempt"?DIALECT_CONNECT_TIMEOUT

redshift is a member of the second and absent from the third. It is in
POSTGRES_WIRE_CLIENTS deliberately (#11389 put it there: it speaks the pg wire
protocol with the same type OIDs and therefore needs the calendar-day parser
pin), and it reaches the server through the pg driver, which honours
connectionTimeoutMillis — but DIALECT_CONNECT_TIMEOUT carries no redshift
row, so withConnectBound's injection is skipped for it.

Why it is not cosmetic

DEFAULT_CONNECT_TIMEOUT_MS (10s) is documented in that method's own header as
"the effective bound", with the 15s pool.createTimeoutMillis a "strictly
looser backstop, reached only by a dialect that has no connect-timeout knob
(SQLite) or ignores the one we set". A redshift host is neither of those: it
has the knob and would honour it. It just never gets it, so it silently falls
through to the looser backstop — the exact failure shape framework#3769 was
filed against (an endpoint that accepts the TCP connection and never completes
the handshake makes every query WAIT rather than fail).

Nothing errors and nothing is logged; the bound is simply 50% looser than the
driver's own docblock says it is, for one client name.

Where measured

packages/drivers/driver-sql/src/sql-driver.tsDIALECT_CONNECT_TIMEOUT and
withConnectBound. Found while working #11550 (converging the dialect-identity
literals), and deliberately not fixed there: that card's dispatch was scoped
to teaching the getters knex's canonical spellings, and adding a redshift row
here is a behaviour change for redshift hosts rather than a mechanical repair.

⚠️ Note for whoever takes this: redshift's absence from this table is
currently load-bearing as documentation. The comment inside
withConnectBound cites it by name as the reason its early return may not skip
the session pins below it ("a return placed at this point silently opted it
out of a fix it needs"). Adding the row invalidates that example, so the comment
needs rewriting in the same change — otherwise the next reader loses the
measurement that explains why the two lists are not one list.

Not the same question as #11756

#11756 asks whether redshift / cockroachdb should be treated as Postgres for
SQL emission. This is independent of that answer: the connect-timeout knob
is a property of the npm driver doing the connecting, not of which DDL dialect
gets compiled, and redshift already has wire recognition today regardless of
how #11756 lands.

Metadata

Metadata

Assignees

No one assigned

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions