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

ReadySet does not handle unsupported parameters issued as TEXT #266

Closed
dfwilbanks395 opened this issue Aug 11, 2023 · 2 comments
Closed
Assignees
Labels
2 points Created by Linear-GitHub Sync High priority Created by Linear-GitHub Sync
Milestone

Comments

@dfwilbanks395
Copy link

dfwilbanks395 commented Aug 11, 2023

Reported by @readyset-railway:

In a Postgres deployment, given the query SELECT * FROM foo WHERE foo.unsupported_type = ?, ReadySet will fail to proxy if the parameter sent in the BIND message uses the Text format.

Related to 111

@dfwilbanks395 dfwilbanks395 added the High priority Created by Linear-GitHub Sync label Aug 11, 2023
@nickelization
Copy link
Contributor

This is kind of also arguably a duplicate of #268 - both issues would be solved if we implemented support for passing through unsupported types in text mode, I think.

Although, not having looked at the code too much yet, I guess it's possible we might solve the issue of passing in unsupported types for parameters separately from the issue of returning unsupported types in query results, so maybe it's still worth keeping this around as a separate issue in case they end up being separate fixes.

@nickelization
Copy link
Contributor

Actually, my previous comment was wrong, this is an entirely independent issue from #268, since the encoding formats for parameters are handled by totally different code than encoding formats for result sets.

@dfwilbanks395 dfwilbanks395 added the 2 points Created by Linear-GitHub Sync label Aug 23, 2023
readysetbot pushed a commit that referenced this issue Aug 29, 2023
This commit adds the new field, but doesn't do anything interesting with
it yet. The plan is to use this to support passthrough of text-formatted
parameters of unsupported types when we proxy queries, but for now we
just hardcode the format to Binary everywhere so as to maintain the
existing behavior.

Followup commits will add support for encoding parameters as text in
proxied queries when the format flag is set to do so, and once that's
done we can actually set the format flag to Text and use the PassThrough
type in the `get_text_value` function in the psql-srv decoder.

Refs: #266
Refs: REA-3183
Change-Id: I3419ba96e1435b30b08f2bd154832da19afd2d59
readysetbot pushed a commit that referenced this issue Aug 29, 2023
The default implementation of this trait method is to use binary format
for everything, and we still do that for almost all cases, but the new
passthrough text format is the one exception.

By implementing this, tokio-postgres will automatically send the right
flags in the Execute message to signify that we're sending text encoding
for any passthrough parameter that has the `format` parameter set to
Text. Since the ToSql impl already just encodes whatever raw data is
held in a DfValue::PassThrough, no further changes are needed to support
passing through text parameters.

Refs: #266
Refs: REA-3183
Change-Id: I04f6fa1e91a153669c72f76cd44e98298a102533
readysetbot pushed a commit that referenced this issue Aug 29, 2023
This ties together the last couple of commits that add support for
sending text passthrough parameters, and actually starts using it so
that we can proxy unsupported types for parameters in text mode.

Fixes: #266
Fixes: REA-3183
Release-Note-Core: Fix proxying of Postgres queries that use the
  Postgres text protocol to send query parameters when those parameters
  use types that aren't natively supported by ReadySet.
Change-Id: Ia9116b0cbd8e237963fce59c543b57f678c5871f
@dfwilbanks395 dfwilbanks395 added this to the v.7 milestone Aug 29, 2023
readysetbot pushed a commit that referenced this issue Aug 31, 2023
This commit adds the new field, but doesn't do anything interesting with
it yet. The plan is to use this to support passthrough of text-formatted
parameters of unsupported types when we proxy queries, but for now we
just hardcode the format to Binary everywhere so as to maintain the
existing behavior.

Followup commits will add support for encoding parameters as text in
proxied queries when the format flag is set to do so, and once that's
done we can actually set the format flag to Text and use the PassThrough
type in the `get_text_value` function in the psql-srv decoder.

Refs: #266
Refs: REA-3183
Change-Id: I3419ba96e1435b30b08f2bd154832da19afd2d59
Reviewed-on: https://gerrit.readyset.name/c/readyset/+/5930
Tested-by: Buildkite CI
Reviewed-by: Dan Wilbanks <dan@readyset.io>
readysetbot pushed a commit that referenced this issue Aug 31, 2023
The default implementation of this trait method is to use binary format
for everything, and we still do that for almost all cases, but the new
passthrough text format is the one exception.

By implementing this, tokio-postgres will automatically send the right
flags in the Execute message to signify that we're sending text encoding
for any passthrough parameter that has the `format` parameter set to
Text. Since the ToSql impl already just encodes whatever raw data is
held in a DfValue::PassThrough, no further changes are needed to support
passing through text parameters.

Refs: #266
Refs: REA-3183
Change-Id: I04f6fa1e91a153669c72f76cd44e98298a102533
Reviewed-on: https://gerrit.readyset.name/c/readyset/+/5931
Tested-by: Buildkite CI
Reviewed-by: Dan Wilbanks <dan@readyset.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 points Created by Linear-GitHub Sync High priority Created by Linear-GitHub Sync
Projects
None yet
Development

No branches or pull requests

2 participants