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

add message body size limits in frontend and backend #2138

Merged
merged 3 commits into from
Oct 5, 2024

Conversation

zenkovev
Copy link

@zenkovev zenkovev commented Oct 2, 2024

No description provided.

@zenkovev
Copy link
Author

zenkovev commented Oct 2, 2024

Refers to #2139

@jackc
Copy link
Owner

jackc commented Oct 3, 2024

I'm not opposed to the underlying idea, but globals aren't the correct approach. You can control the frontend creation via the BuildFrontend function in pgconn.Config. And you can also directly access the frontend via https://pkg.go.dev/github.com/jackc/pgx/[email protected]/pgconn#PgConn.Frontend. So any setting should be controlled via one of those means. Not a global with atomics.

@zenkovev
Copy link
Author

zenkovev commented Oct 4, 2024

Thanks for the reply. Regarding the control over the frontend creation, yes, it's fair, then global atomics are really not needed. There remains control over the length of the frontend message at the structure level - PR has been updated.

@jackc
Copy link
Owner

jackc commented Oct 5, 2024

The Frontend changes look okay. Seem to be just porting the existing system from the backend. But I'm not sure why there are any changes on Backend.

Also, the comment style of having the comments below the field instead of above or beside looks odd to me (Frontend.maxBodyLen).

@zenkovev
Copy link
Author

zenkovev commented Oct 5, 2024

I fixed the comments. About the changes in the Backend: there is one change transferred from the Frontend, checking that a non-negative number is set in b.bodyLen, that is, that the specified length of the message is correct.

@jackc jackc merged commit cc05954 into jackc:master Oct 5, 2024
14 checks passed
@jackc
Copy link
Owner

jackc commented Oct 5, 2024

👍

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