Skip to content

Implemented a simplified message framework#1

Closed
Hydrocharged wants to merge 3 commits intomainfrom
daylon/message-structure
Closed

Implemented a simplified message framework#1
Hydrocharged wants to merge 3 commits intomainfrom
daylon/message-structure

Conversation

@Hydrocharged
Copy link
Copy Markdown
Collaborator

This makes it so that declaring PostgreSQL messages is almost as straightforward as copying them from the document into Go, as the framework takes care of transforming the data into the format that PostgreSQL expects. This will significantly speed up adding new messages, while adding a layer of validation to ensure that some mistakes aren't possible.

@Hydrocharged Hydrocharged force-pushed the daylon/message-structure branch from 99afe05 to d3e3924 Compare September 19, 2023 16:43
@Hydrocharged
Copy link
Copy Markdown
Collaborator Author

The best way to review this:

  1. Ignore the main.go file. It's just in a spot where it runs, it was basically copied and modified from Dolt to give it that "default run server mode" implementation. I'll make an actual main.go file later.
  2. listener.go shows how the main listening loop works. We're sending and receiving PostgreSQL messages.
  3. All Go files that being with message are worth reviewing. This is the bulk of this PR's complexity. Rather than implementing custom serialization-deserialization logic for every message type (which is what I had) where it was easy to make a mistake and not necessarily straightforward on how to approach each message, this implements a simple interface where you define a "default" message by essentially copying the layout from the documentation. From there, you write to this layout or read from this layout, and everything is named so you don't need to remember indices, etc. There's a lot of validation to ensure that the message format is correct, and that it's all being used correctly, so it's relatively fool-proof. That does mean that Message is handling a lot of stuff, but the complexity is in one location rather than being spread out, and can account for corner-cases that might not be evident for any one message. Plus, I think it's significantly easier to visually parse and understand the messages now.
  4. All other non-message... files in the messages folder are the actual messages, which lets you see how they're implemented. They're copied from this document.

Taking this approach, you don't actually need to review all 2,000 lines since a lot of it consists of message definitions.

@Hydrocharged Hydrocharged requested a review from zachmu September 20, 2023 14:10
@Hydrocharged
Copy link
Copy Markdown
Collaborator Author

99% of this would be written to verify messages at compile-time if this were C++, but Go doesn't believe in compile-time behavior except for allowing certain files on certain platforms 😭

Copy link
Copy Markdown
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks pretty clean, comments are pretty minor.

The init() blocks are nasty. Are they strictly necessary?

}

// Send sends the given message over the connection.
func Send(conn net.Conn, message MessageType) error {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems misplaced. Probably better to have Encode(message) []byte and keep the writing / connection logic out of this package

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of making a connection package, so I might just do that instead. Also having the message definitions in there. Since PostgreSQL seems to strictly communicate in messages, there's no reason for the listener to deal with byte slices when it'll just be sending and receiving the messages, so I want the interface to handle that. This also means that any other edge cases, like dynamically making multiple requests as the byte buffer isn't big enough, is hidden behind the call.

@Hydrocharged
Copy link
Copy Markdown
Collaborator Author

Merging the feedback from this PR into #2, so closing this one.

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