cli/sql: avoid session reconnect upon query cancellation

[knz]

Describe the problem

This is a followup to #76437, especially the problem noticed during testing: a bug in lib/pq forces the connection to be dropped.
Discussed with @rafiss we have two solutions

• improve lib/pq to fix the bug
• use pgx instead.

To Reproduce

run a long query and interrupt it interactively.

Expected behavior

The session remains open, including all its session variable customizations.

Jira issue: CRDB-13144

[knz]

@rafiss I did some research into using pgx instead of lib/pq.

The APIs are certainly nicer and cleaner. Unfortunately, pgx does not yet support GSSAPI authentication, which some of our customers have become dependent on.

So we have several possible choices forward:

• add kerberos support to pgx. The implementation in lib/pq is very small, so maybe it'll be small in pgx too.
• fix lib/pq to remove the offending bug. In that upstream PR, mjibson suggested he's open to reverting the change if there's a unit test to demonstrate the problem and prevent a regression.
• as you suggested earlier, expose the query cancel method as a public API in lib/pq, then change our CLI Code to use that. It woulnd't be as elegant/compact as context-based cancellation, but 🤷

WDYT?

[rafiss]

I think it would be great if pgx had kerberos support; that's my first choice. tracking issue for it: jackc/pgx#1166

[knz]

Thanks sounds good. Are we providing incentives for that to happen?

[rafiss]

Thanks sounds good. Are we providing incentives for that to happen?

We're discussing that with the DX team.

[erikgrinaker]

I think we should do something about this before we ship 22.1, the current Ctrl-C behavior is not a good look:

^C
attempting to cancel query...
ERROR: query execution canceled
SQLSTATE: 57014
warning: error retrieving the transaction status: driver: bad connection
warning: connection lost!
opening new connection: all session settings will be lost
root@:26257/defaultdb ?>

[knz]

IMHO it's objectively better than the previous look, where the SQL shell would simply exit. What do you think?

[erikgrinaker]

IMHO it's objectively better than the previous look, where the SQL shell would simply exit. What do you think?

For sure, but this makes it look like something is broken or wrong. Maybe we at least can make the error message a bit more friendly or explanatory?

[knz]

What would be a good example output?

[erikgrinaker]

Dropping the two warning messages would go a long way, I think.

[knz]

NB: in #79629, @otan is modifying lib/pq to support the copy protocol in a way the sql shell uses it. However, in this issue @rafiss is recommending we switch over from lib/pq to pgx to benefit from better support for query cancellation (pending support for k5s authn in pgx).

How can we reconcile the two efforts? If we go towards pgx, then @otan's efforts in lib/pq will not help; how can we ensure \copy will continue to work when we switch over to pgx?

[otan]

i'd like COPY to be (if desirable) merged into 22.1, so at least we make progress there.

we'd want to expose pgconn for continued support for COPY; i think this is still aligned here but may slightly make the direction a little difficult (though i'm sure pgx/v4 uses pgconn and so we can perhaps look at exposing that to render access to COPY).

[knz]

i'd like COPY to be (if desirable) merged into 22.1, so at least we make progress there.

no objection. My question above is to determine what we'll do post-release.

we'd want to expose pgconn for continued support for COPY; i think this is still aligned here but may slightly make the direction a little difficult (though i'm sure pgx/v4 uses pgconn and so we can perhaps look at exposing that to render access to COPY).

that's indeed what i had in mind too, I just hope we can bring that point across with the pgx maintainers.

[knz]

Dropping the two warning messages would go a long way, I think.

I've tried doing that, but the resulting UX is still not good; a session reconnect without warning is confusing, because the user loses their customization.

I think it'll be just better to remove the cancellation altogether until we fix the pq bug (or move to pgx). Doing this in #79739.