Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add SslResponse message #117

Merged
merged 2 commits into from
Oct 22, 2023

Conversation

elmaxxo
Copy link
Contributor

@elmaxxo elmaxxo commented Oct 20, 2023

Hello again! I'd like to introduce a new backend message.

The backend should communicate with the frontend using backend messages. But this simple concept is broken when you want to response to SslRequest messsage. There is no such message as SslResponse in postgres, so to answer to SslRequest they use a very special approach: the backend sends a single byte containing 'S' or 'N', indicating that it is willing or unwilling to perform SSL, respectively.

I don't like how this is done in postgres and, as for me, it is very unnatural, so this patch introduces SslResponse message, that simply wraps this single byte and provides the backend with convenient API for responding to an SslRequest message.

Also it is better in terms of symmetry: there is SslRequest in frontend messages, but there is no SslResponse in backend messages.

The backend should communicate with the frontend using backend messages.
But this simple concept is broken when you want to response to
SslRequest messsage. There is no such message as SslResponse in
postgres, so to answer to SslRequest they use a very special approach:
the backend sends a single byte containing 'S' or 'N',
indicating that it is willing or unwilling to perform SSL, respectively.

I don't like how this is done in postgres and, as for me, it is very
unnatural, so this patch introduces SslResponse message, that simply
wraps this single byte and provides the backend with convenient API
for responding to an SslRequest message.

Also it is better in terms of symmetry: there is SslRequest in frontend
messages, but there is no SslResponse in backend messages.
@elmaxxo elmaxxo force-pushed the elmaxxo/feat/add-ssl-response-message branch from d72a72a to 5312138 Compare October 20, 2023 17:05
@sunng87
Copy link
Owner

sunng87 commented Oct 21, 2023

No problem @elmaxxo . The reason why I didn't included SslResponse is I found it's weird too. I'm OK to have it in our codec, but you may want to update pgwire::tokio::peek_for_sslrequest as well to use the message type.

@elmaxxo elmaxxo force-pushed the elmaxxo/feat/add-ssl-response-message branch 6 times, most recently from 52dc5ef to 3c72a82 Compare October 21, 2023 21:57
@elmaxxo elmaxxo force-pushed the elmaxxo/feat/add-ssl-response-message branch from 3c72a82 to 6e653e8 Compare October 21, 2023 22:31
@elmaxxo
Copy link
Contributor Author

elmaxxo commented Oct 21, 2023

Thank you for highlighting pgwire::tokio::peek_for_sslrequest. I have refactored it, please take a look.

I relized that there is no simple way to peek a message, as opposed to reading it. Don't you think that SslRequest should be handled in SimpleQueryHandler::on_startup? It would allow us to read such messages as well as Startup and handle them in place.

Also, shouldn't we handle CancelRequest?

@sunng87
Copy link
Owner

sunng87 commented Oct 22, 2023

  1. Good idea. Remember to keep a default implementation so that most developer won't need to know the detail of SSL handshake.
  2. The whole cancel functionality is not implemented at the moment. It has a different pattern with default startup process. PR is welcomed but let me know your design before you get started.

@sunng87 sunng87 merged commit 6ff434c into sunng87:master Oct 22, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants