ParameterStatus on SET statements

[TwistingTwists]

Attempt to fix: #242

[TwistingTwists]

I fixed the cargo fmt issue. Can you trigger the CI please @sunng87 ?

[sunng87]

Thank you @TwistingTwists ! Please check my comments about moving the logic into our QueryHook completely.

[sunng87]

I haven't checked the order of returned packets for SET statements, I wonder if it's possible to add this logic to set_show's try_respond_set_statements. We can modify the definition of client in QueryHook if needed.

[TwistingTwists]

Modifying the client definition to give hooks Sink access would require either:

• Generic methods on QueryHook → breaks object safety (Vec<Arc<dyn QueryHook>>)
• A wrapper trait over ClientInfo → 12+ method delegation, fragile across pgwire upgrades

Instead, I'd propose making HookOutput a #[non_exhaustive] builder struct — hooks return what they want sent (ParameterStatus, and notices - in future), and do_query handles the actual client.feed(). This keeps all SET→ParameterStatus logic in set_show.rs while preserving object safety.

Adding future fields (e.g., notices for warnings) becomes non-breaking. This matches how Tower/Axum middleware returns response data rather than writing to the connection directly.

An impl is here:
359d428

I am open to take the other approach. (will revert this commit)
Let me know your thoughts @sunng87

[sunng87]

hi @TwistingTwists , I think we can just add Sink<PgWireBackendMessage> to our handler implementation and QueryHook definition, because the original SimpleQueryHandler and ExtendedQueryHandler has Sink<PgWireBackendMessage> for the C parameter.

I still hope you can just call client.send() or client.feed() in set_show as a side-effect, instead of returning it as response of QueryHook. Because there can be a chance we will need to deal with all possible messages in HookOutput, then it becomes duplicated information and fragile to maintain.

[TwistingTwists]

hooks take &mut (dyn ClientInfo + Send + Sync). The simplest attempt is to add Sink<PgWireBackendMessage> to that dyn bound.

We get this error.

error[E0225]: only auto traits can be used as additional traits in a trait object
  --> datafusion-postgres/src/hooks/mod.rs:34:36
   |
34 | ...yn ClientInfo + Sink<PgWireBackendMessage> + ...
   |       ----------   ^^^^^^^^^^^^^^^^^^^^^^^^^^ additional non-auto trait
   |       |
   |       first non-auto trait
   |

I agree that managing HookOutput can be a little fragile.

Alternatively, we can have a supertrait -> https://github.com/datafusion-contrib/datafusion-postgres/pull/293/changes#diff-89ae6cc475596662d15f32aeafc00d7f0122f4398143dc26d481e76f20fe83e1R23

[sunng87]

I see. Thank you for the information. Let me experiment by myself.

[sunng87]

hello @TwistingTwists , could you try if this HookClient will work?

#[async_trait]
pub trait HookClient: ClientInfo + Send + Sync {
    async fn send_message(&mut self, item: PgWireBackendMessage) -> PgWireResult<()>;
}

#[async_trait]
impl<S> HookClient for S
where
    S: ClientInfo + Sink<PgWireBackendMessage> + Send + Sync + Unpin,
    PgWireError: From<<S as Sink<PgWireBackendMessage>>::Error>,
{
    async fn send_message(&mut self, item: PgWireBackendMessage) -> PgWireResult<()> {
        use futures::SinkExt;
        self.send(item).await.map_err(PgWireError::from)
    }
}

It has auto implementation of ClientInfo and a simple manually implementation for Sink

[TwistingTwists]

Let me give that a spin.

[TwistingTwists]

Thanks for this direction!

@sunng87 : This does remove the generic here: https://github.com/datafusion-contrib/datafusion-postgres/pull/293/changes#diff-9102592acfefe305e66a99d1eae41fff202430bff0f5d7a8c8082255966a604aL121

Should be fine, i guess.

code now works.

[sunng87]

Awesome. Thank you @TwistingTwists