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

pg_aggregate needs a makeover #1362

Open
workingjubilee opened this issue Oct 28, 2023 · 2 comments
Open

pg_aggregate needs a makeover #1362

workingjubilee opened this issue Oct 28, 2023 · 2 comments
Labels
build-sys Regarding the "build system" of pgrx which is split between cargo-pgrx, pg-sys, and sql-entity-graph ffi-safety macro rust+sql Interop between Rust and SQL sql-gen sql-safety

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Oct 28, 2023

Initially inspired from #1357 and its discoveries.

Other issues related to aggregates specifically:

Related to all SQL generation:

@workingjubilee workingjubilee changed the title pg_aggregate needs to be audited for soundness pg_aggregate needs a makeover Feb 12, 2024
@workingjubilee workingjubilee added sql-safety sql-gen rust+sql Interop between Rust and SQL macro build-sys Regarding the "build system" of pgrx which is split between cargo-pgrx, pg-sys, and sql-entity-graph labels Feb 12, 2024
@workingjubilee
Copy link
Member Author

effectively blocked on #1661

@workingjubilee
Copy link
Member Author

Lifted from #1731:

The problem isn't with Aggregate, per se, it's with certain types.

These are aggregates that are supposed to manage composite types. But we know composite types are allocated in Postgres, thus are effectively lifetime-bound. But #[pg_aggregate] and Aggregate don't know how to convey the lifetimes through. Thus even annotating it with something like this does not actually fix the problem:

#[pg_aggregate]
impl<'a> Aggregate for SumScritches<'a> {
    type State = i32;
    const INITIAL_CONDITION: Option<&'static str> = Some("0");
    type Args = pgrx::name!(value, pgrx::composite_type!('a, "Dog"));

    fn state(
        current: Self::State,
        arg: Self::Args,
        _fcinfo: pg_sys::FunctionCallInfo,
    ) -> Self::State {
        todo!()
    }
}

Thus in order to make aggregates work for complex cases we must break the world for them as well, making all their functions use the correct, lifetime-bound types. Hopefully this will let us simplify the code expansion for them as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-sys Regarding the "build system" of pgrx which is split between cargo-pgrx, pg-sys, and sql-entity-graph ffi-safety macro rust+sql Interop between Rust and SQL sql-gen sql-safety
Projects
None yet
Development

No branches or pull requests

1 participant