sunng87 / pgwire

PostgreSQL wire protocol implemented as a rust library.
Apache License 2.0
531 stars 40 forks source link

ExtendedQueryHandler question #82

Closed osawyerr closed 1 year ago

osawyerr commented 1 year ago

Hi there. I'm trying to override ExtendedQueryHandler::on_execute() I'm basing my implementation on something similar to the default implementation. However I don't have access to QueryResponse::data_rows and QueryResponse::row_schema because they are pub(crate).

Was this intentional? If so, how can we get access to data_rows so we can iterate through it?

#[derive(Getters)]
#[getset(get = "pub")]
pub struct QueryResponse<'a> {
    pub(crate) row_schema: Arc<Vec<FieldInfo>>,
    pub(crate) data_rows: BoxStream<'a, PgWireResult<DataRow>>,
}
sunng87 commented 1 year ago

hi @osawyerr , there are public getters for these fields, you can access with .row_schema()

osawyerr commented 1 year ago

Yes I saw those. But they return & and its not clear how to convert a &BoxStream to a BoxStream for the data_rows, for example.

I tried using as_ref() but then you cant call next() on what that returns:

let mut data_rows = results.data_rows().as_ref();
while let Some(row) = data_rows.next().await {
...
226 |     while let Some(row) = data_rows.next().await {
    |                           ^^^^^^^^^ ---- required by a bound introduced by this call
    |                           |
    |                           the trait `Unpin` is not implemented for `dyn Stream<Item = Result<pgwire::messages::data::DataRow, PgWireError>> + std::marker::Send`

This doesn't work either:

let mut data_rows = results.data_rows();
while let Some(row) = data_rows.next().await {
...
228 |     while let Some(row) = data_rows.next().await {
    |                           ^^^^^^^^^^^^^^^^ `data_rows` is a `&` reference, so the data it refers to cannot be borrowed as mutable
sunng87 commented 1 year ago

I see. I'm on vocation today and will look into this issue tonight. Perhaps we can add owned version of getters for both fields.

sunng87 commented 1 year ago

By the way, I just exposed send_query_response in 0.13.1 I'm not sure if it's what you need.

osawyerr commented 1 year ago

Yes thats working well thanks. You might consider making the other helper methods public as well to make defining a ExtendedQueryHandler easier - send_execution_response, send_describe_response, etc

Also I have a question, how can we force the ExtendedQueryHandler to always be called? I have defined it and added it to process_socket but the SimpleQueryHandler is being called when I send a query select * from my_table. Is there a way to disable this?

Just for some background, the reason I'm doing this is I want to call a method foo() before the do_query() method is executed (it can't be called in the body of do_query()). Thats why I am writing an ExtendedQueryHandler so that I can call foo() in on_execute. For further context,foo() returns an object that needs to be alive when do_query() completes. But because do_query() streams, foo() cannot be called in do_query(). It needs to be called before do_query()

I'm not sure if using an ExtendedQueryHandler is the best way to achieve this.

sunng87 commented 1 year ago

Also I have a question, how can we force the ExtendedQueryHandler to always be called? I have defined it and added it to process_socket but the SimpleQueryHandler is being called when I send a query select * from my_table. Is there a way to disable this?

It depends on what type of subquery the client uses. For example, psql always uses simple subprotocol and sends query in plain text, thus SimpleQueryHandler are used to respond it. Clients like JDBC use extended subprotocol by default so ExtendedQueryHandler are called.

I assume you are using foo() to setup some resource to support do_query(). Is it possible to call foo() upon the initialization of SimpleQueryHandler and ExtendedQueryHandler. Both handlers can be implemented on same object. And they are initialized for each client connection.

osawyerr commented 1 year ago

Ah that makes sense. I'm using psql for my tests.

I assume you are using foo() to setup some resource to support do_query(). Is it possible to call foo() upon the initialization of SimpleQueryHandler and ExtendedQueryHandler. Both handlers can be implemented on same object. And they are initialized for each client connection.

Yes its setting up a resource required for do_query(). Hmmm, I did consider setting the resource up when SimpleQueryHandler and ExtendedQueryHandler are initialised as you mentioned. However, those are initialised each time a client connects for the lifetime of the connection. But the requirement is that the resource needs to be initialised on a per query basis.

sunng87 commented 1 year ago

OK. Then I think you will need to override on_query from SimpleQueryHandler and on_execute from ExtendedQueryHandler for your case.