pgcentralfoundation / pgrx

Build Postgres Extensions with Rust!
Other
3.64k stars 248 forks source link

Relax bounds on aggregate functions? #1272

Open marcusllorente-msft opened 1 year ago

marcusllorente-msft commented 1 year ago

Hi - new to Rust and PGRX, so excuse me if my questions are a bit silly. I am trying to create an aggregate function in PGRX and I am working with the aggregate example code to get a better understanding. My goal right now is to read a column that contains text in PGSQL and to insert each piece of text into a vector that is a field in the aggregate state. However, when trying to initialize the field in the state struct, it gives me issues with some of the traits and types. This is the code I am working with now - it is the same as the aggregate example but I am just adding a vector field inside the struct to continue with testing.

use core::ffi::CStr;
use pgrx::aggregate::*;
use pgrx::prelude::*;
use pgrx::{pgrx, PgVarlena, PgVarlenaInOutFuncs, StringInfo};
use serde::{Deserialize, Serialize};
use std::str::FromStr;

pgrx::pg_module_magic!();

#[derive(Copy, Clone, PostgresType, Serialize, Deserialize)]
#[pgvarlena_inoutfuncs]
pub struct IntegerAvgState {
    sum: i32,
    n: i32,
    inputs: Vec<String>,
}
(The rest of the code is copied and pasted from the aggregates lib.rs code)

When attempting to run, I get this error: 19 | #[derive(Copy, Clone, PostgresType, Serialize, Deserialize)] | ^^^^ ... 24 | inputs: Vec<String>, | ------------------- this field does not implementstd::marker::Copy``

In reference to the inputs field I added in the struct. Any feedback would be super appreciated. Thank you so much!

eeeebbbbrrrr commented 1 year ago

Vev<String> doesn't derive Copy, so your IntegerAvgState struct can't either. That's just a Rust thing.

I don't believe pgrx' aggregate support requires the state function to derive Copy, so you should be able to remove it.

marcusllorente-msft commented 1 year ago

When I tried to remove it, it errors saying that the trait bound IntegerAvgState: std::marker::Copy is not satisfied with the following details:

97  | pub struct PgVarlena<T>
    |            --------- required by a bound in this struct
98  | where
99  |     T: Copy + Sized,
    |        ^^^^ required by this bound in `PgVarlena`
eeeebbbbrrrr commented 1 year ago

Okay, upon further investigation, our aggregate state function is bound by Copy. My bad. I can't tell you why off the top of my head tho.

Basically, that means your "state" struct needs to be a fixed size and not contain anything that also does heap allocation, such as a Vec (or a String or a HashMap, etc ,etc).

It's not clear to me what you're trying to accomplish by storing inputs: Vec<String>, but perhaps you can find another approach?

marcusllorente-msft commented 1 year ago

Ahhh, the fixed size constraint makes sense. I'm gonna see if I can try to find a work around. Thank you for your help!

workingjubilee commented 1 year ago

After I finish with the List rework I will take a look at aggregates. Unless someone beats me to it.

workingjubilee commented 1 year ago

Investigating this is waiting for the new MemCx API to land, because the relaxation won't be sound otherwise.