Fixes LISTEN to quote channel name

[NeedleInAJayStack]

This adjusts the SQL LISTEN statement called by PostgresConnection.addListener to quote the channel name.

For example, previously addListener("abc") would run LISTEN abc; and it will now run LISTEN "abc";

As far as I can tell, this only matters when the channel name is a reserved Postgres keyword, like default.

[NeedleInAJayStack]

This behavior appears to have been introduced in v1.18.0. Interestingly, by watching SQL logs it appears that version also changed addListener to call LISTEN automatically, so some of the notification tests are calling LISTEN twice.

[fabianfett]

Nice catch! See comments!

[fabianfett]

Suggested change

	query: PostgresQuery(unsafeSQL: "LISTEN \"\(channel)\";"),
	query: PostgresQuery(unsafeSQL: #"LISTEN "\#(channel)";"),

[NeedleInAJayStack]

Done, thanks!

[fabianfett]

Suggested change

	query: PostgresQuery(unsafeSQL: "UNLISTEN \"\(channel)\";"),
	query: PostgresQuery(unsafeSQL: #"UNLISTEN "\#(channel)";"#),

[NeedleInAJayStack]

Good call. Done

[fabianfett]

How does this test interact with the code that you changed? Maybe we want to change AsyncPostgresConnectionTests.testListenAndNotify to something like this:

    func testListenAndNotify() async throws {
        let channelNames = [
            "foo",
            "default"
        ]

        let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1)
        defer { XCTAssertNoThrow(try eventLoopGroup.syncShutdownGracefully()) }
        let eventLoop = eventLoopGroup.next()

        for channelName in channelNames {
            try await self.withTestConnection(on: eventLoop) { connection in
                let stream = try await connection.listen(channelName)
                var iterator = stream.makeAsyncIterator()

                try await self.withTestConnection(on: eventLoop) { other in
                    try await other.query(#"NOTIFY "\#(unescaped: channelName)", 'bar';"#, logger: .psqlTest)

                    try await other.query(#"NOTIFY "\#(unescaped: channelName)", 'foo';"#, logger: .psqlTest)
                }

                let first = try await iterator.next()
                XCTAssertEqual(first?.payload, "bar")

                let second = try await iterator.next()
                XCTAssertEqual(second?.payload, "foo")
            }
        }
    }

[NeedleInAJayStack]

My change was related to the quoting that occurs in the LISTEN SQL query caused by addListener. So before the changes, line 237 of this new test would fail on the PostgreSQL side when running LISTEN default; with the error:

ERROR: syntax error at or near "default" at character 8
STATEMENT: LISTEN default;

[NeedleInAJayStack]

I'm totally happy to combine it into testListenAndNotify though if you prefer it that way.

[NeedleInAJayStack]

Cool, I've made your recommended changes to testListenAndNotify and removed the testNotificationWithKeywordChannelName test

[fabianfett]

This behavior appears to have been introduced in v1.18.0. Interestingly, by watching SQL logs it appears that version also changed addListener to call LISTEN automatically, so some of the notification tests are calling LISTEN twice.

@NeedleInAJayStack Does this mean you actually used the old LISTEN API?

[NeedleInAJayStack]

Does this mean you actually used the old LISTEN API?

Yeah, we were using the old LISTEN API. However, my comment was more related to these tests, and specifically that the lines with conn?.simpleQuery("LISTEN example").wait() are technically no longer needed since addListener now calls LISTEN example;. When watching the SQL logs I could see two LISTEN example queries in a row.

[NeedleInAJayStack]

I'm able to recreate the TinyFastSequenceTests.testReserveCapacityIsForwarded CI failures in a swift:5.10-jammy container locally:

/repo/Tests/ConnectionPoolModuleTests/TinyFastSequenceTests.swift:37: error: TinyFastSequenceTests.testReserveCapacityIsForwarded : XCTAssertEqual failed: ("9") is not equal to ("8") - 
/repo/Tests/ConnectionPoolModuleTests/TinyFastSequenceTests.swift:46: error: TinyFastSequenceTests.testReserveCapacityIsForwarded : XCTAssertEqual failed: ("9") is not equal to ("8") -
/repo/Tests/ConnectionPoolModuleTests/TinyFastSequenceTests.swift:53: error: TinyFastSequenceTests.testReserveCapacityIsForwarded : XCTAssertEqual failed: ("9") is not equal to ("8") -

I'm able to recreate these errors on main, and going back to v1.19.0 where these tests were introduced. I'm not sure why the CI passed on previous MRs though. Do you guys have any idea what might be different now compared with those other MRs?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.73%. Comparing base (8f8724e) to head (12c5d5d).

Additional details and impacted files

@@            Coverage Diff            @@
##           main     #466       +/-   ##
=========================================
+ Coverage      0   61.73%   +61.73%     
=========================================
  Files         0      125      +125     
  Lines         0    10024    +10024     
=========================================
+ Hits          0     6188     +6188     
- Misses        0     3836     +3836     

Files	Coverage Δ
...urces/PostgresNIO/New/PostgresChannelHandler.swift	84.31% <100.00%> (ø)

... and 124 files with indirect coverage changes