Make backend key data optional

[rausnitz]

This allows the ConnectionStateMachine to be in state readyForQuery even if the BackendKeyData has not been received.

The use case that prompted this change is integration with Amazon RDS Proxy. When connecting to Postgres via RDS Proxy, any query fails with error PSQLError(base: PostgresNIO.PSQLError.Base.unexpectedBackendMessage(.readyForQuery(.idle))).

According to the RDS Proxy docs, one limitation of RDS Proxy is "For PostgreSQL, RDS Proxy doesn't currently support canceling a query from a client." The Postgres docs explain BackendKeyData's purpose as: "This message provides secret-key data that the frontend must save if it wants to be able to issue cancel requests later. The frontend should not respond to this message, but should continue listening for a ReadyForQuery message."

So, it seems that RDS Proxy for Postgres has no use for BackendKeyData and does not provide it.

Allowing nil for BackendKeyData in this function resolves the PSQLError error we were seeing.

[fabianfett]

Thanks for bringing this up! I think this is something that we should definitely support. However, I think, we should make all of this way more explicit than currently proposed:

1. We should add a setting to PostgresConnection.Configuration.Conntection that allow connections to not provide BackendKeyData. A possible name should be: allowNotProvidingBackendKeyData. By default this property should be set to false.
2. We need to inject the value of the property into the StateMachine.
3. If we receive readyForQuery without having received backendKeyData before, we should check, if the property allowNotProvidingBackendKeyData is true and only progress in this case.
4. We need unit test cases that cover both flows.

Is this something you feel comfortable tackling? I'm here to help you at any step on the way.

[rausnitz]

Can do. I'll work on those changes. Thanks!

[rausnitz]

I made some changes so that connections must provide BackendKeyData by default. If this approach is acceptable, I'll add unit tests.

[rausnitz]

Actually, handling this in the unit tests was straightforward, so I went ahead and updated it.

[fabianfett]

This already looks really good. Don't get distracted by the amount of code comments... This is awesome work!

Would you mind addressing the comments and add a test case for the new code path in ConnectionStateMachineTests.swift?

Two examples tests are here:

    func testBackendKeyAndParameterStatusReceivedAfterAuthenticated() {
        var state = ConnectionStateMachine(.authenticated(nil, [:]))
        
        XCTAssertEqual(state.backendKeyDataReceived(.init(processID: 2730, secretKey: 882037977)), .wait)
        
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "DateStyle", value: "ISO, MDY")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "application_name", value: "")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "server_encoding", value: "UTF8")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "integer_datetimes", value: "on")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "client_encoding", value: "UTF8")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "TimeZone", value: "Etc/UTC")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "is_superuser", value: "on")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "server_version", value: "13.1 (Debian 13.1-1.pgdg100+1)")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "session_authorization", value: "postgres")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "IntervalStyle", value: "postgres")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "standard_conforming_strings", value: "on")), .wait)
        
        XCTAssertEqual(state.readyForQueryReceived(.idle), .fireEventReadyForQuery)
    }
    
    func testReadyForQueryReceivedWithoutBackendKeyAfterAuthenticated() {
        var state = ConnectionStateMachine(.authenticated(nil, [:]))
        
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "DateStyle", value: "ISO, MDY")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "application_name", value: "")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "server_encoding", value: "UTF8")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "integer_datetimes", value: "on")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "client_encoding", value: "UTF8")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "TimeZone", value: "Etc/UTC")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "is_superuser", value: "on")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "server_version", value: "13.1 (Debian 13.1-1.pgdg100+1)")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "session_authorization", value: "postgres")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "IntervalStyle", value: "postgres")), .wait)
        XCTAssertEqual(state.parameterStatusReceived(.init(parameter: "standard_conforming_strings", value: "on")), .wait)
        
        XCTAssertEqual(state.readyForQueryReceived(.idle),
                       .closeConnectionAndCleanup(.init(action: .close, tasks: [], error: PSQLError.unexpectedBackendMessage(.readyForQuery(.idle)), closePromise: nil)))
    }

Again, thanks a lot for driving this forward...

[codecov-commenter]

Codecov Report

Merging #296 (d2ee24c) into main (2825829) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##             main     #296   +/-   ##
=======================================
  Coverage   43.91%   43.91%           
=======================================
  Files         115      115           
  Lines        9678     9680    +2     
=======================================
+ Hits         4250     4251    +1     
- Misses       5428     5429    +1     

Flag	Coverage Δ
unittests	43.91% <66.66%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...es/PostgresNIO/Connection/PostgresConnection.swift	17.14% <33.33%> (+0.14%)	⬆️
...nection State Machine/ConnectionStateMachine.swift	57.53% <75.00%> (ø)
...urces/PostgresNIO/New/PostgresChannelHandler.swift	61.14% <100.00%> (ø)

[fabianfett]

This looks really good now. Only thing left is merge of main into your branch!

[rausnitz]

This looks really good now. Only thing left is merge of main into your branch!

Done!

[fabianfett]

One Nit remaining...

[fabianfett]

Awesome patch @rausnitz! Thanks a ton!

[rausnitz]

Thanks for the thorough and fast reviews @fabianfett!