Skip to content

Expose SimpleQueryRow internals #3

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

Merged
merged 1 commit into from
Oct 7, 2022
Merged

Expose SimpleQueryRow internals #3

merged 1 commit into from
Oct 7, 2022

Conversation

dwlbnks
Copy link

@dwlbnks dwlbnks commented Sep 13, 2022

Adds a function to expose the DataRowBody of a SimpleQueryRow. Also adds public constructors for OwnedField and DataRowBody, and makes the SimpleQueryRow constructor public.

The constructors are useful for integration testing since these structs are all available outside the module.

Adds a function to expose the DataRowBody of a SimpleQueryRow. Also adds
public constructors for OwnedField and DataRowBody, and makes the
SimpleQueryRow constructor public.

The constructors are useful for integration testing since these structs
are all available outside the module.
@dwlbnks
Copy link
Author

dwlbnks commented Sep 13, 2022

These changes were left out of the last PR because they expose some postgres-protocol internals that might not be appreciated by the upstream maintainers.

Exposing the body of the SimpleQueryRow allows us to perform passthrough more efficiently. Without this we would instead read each value from the SimpleQueryRow and re-serialize them ourselves.

The public constructors allow us to write serialization unit tests. Without these constructors we would be forced to write cumbersome integration tests.

If we make a push to merge our changes upstream, we could consider working without this CL.

@glittershark
Copy link

generally I would like to avoid getting us in a permanent non-upstreamable situation. Maybe we can ask upstream if they'd be willing to merge something like this?

@dwlbnks
Copy link
Author

dwlbnks commented Sep 13, 2022

I would say it doesn't seem promising based on this issue, but I can ask if it would be okay in a feature flag.

@dwlbnks
Copy link
Author

dwlbnks commented Sep 13, 2022

Started a discussion here.

@dwlbnks dwlbnks merged commit 1e4c8fb into master Oct 7, 2022
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