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

Implement changes to support passing through raw query result rows #6

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

nickelization
Copy link

These two commits are necessary for passing through the raw data rows for proxied queries when using the Parse/Bind/Describe/Exec path (as opposed to the SimpleQuery path, where this kind of passthrough is already implemented).

The bulk of the necessary changes are to let us specify custom result formats when we execute a query, since we need to specify the same result formats as the original client query if we want the results we pass through to match what the client expects. We also add a public accessor for the underlying row data on the Row type. See the individual commit messages in this PR for more detail.

(For more context on why I'm making these changes, see readysettech/readyset#268).

This adds the same `body` method we added for SimpleQueryRow, but for
Row this time.
The existing frontend::bind function does allow us to specify arbitrary
result formats, but the query::encode_bind function calls it with a
hardcoded result format of Some(1), which causes us to request binary
format for all results.

Since generic_query_raw is already part of our custom patchset that
we've added on top of the upstream tokio-postgres code, I've modified
this function to take result formats directly, and we can change the
ReadySet code that uses this accordingly. Elsewhere, I've tried to avoid
changing any existing APIs so as to make it easier to maintain this
patch set in the future if we decide to merge in changes from upstream.
As such, I've generally erred on the side of adding new functions that
take the extra argument for result formats, and then retain the existing
functions as-is and have them call the new function with the same
hardcoded Some(1) value that retains the prior behavior.
@nickelization
Copy link
Author

I think I'm likely going to need to add one more commit to this PR, because if we want to add a new variant to psql_srv::message::BackendMessage to wrap a Row, then we may need Row to implement PartialEq since it's derived for BackendMessage. Hoping there's a way around that, since we only seem to use the PartialEq implementation for tests, which wouldn't be using the variant that wraps a Row anyway, but it'd add a bunch of ugly boilerplate to do a custom impl so I'm weighing my options right now.

Copy link

@dfwilbanks395 dfwilbanks395 left a comment

Choose a reason for hiding this comment

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

This part lgtm, but if you end up adding another commit you can re-request

@nickelization
Copy link
Author

Cool, thanks @dfwilbanks395! I ultimately decided it's better to eliminate the PartialEq impl on BackendMessage instead, so I think these changes should be sufficient after all.

(I think we can also revert some of the derives we added in 9366b33 now that we don't need them for BackendMessage anymore, but I'll address that later in a separate PR.)

@nickelization nickelization merged commit 530ae67 into master Aug 22, 2023
0 of 3 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