Skip to content

Commit

Permalink
Fix lifetimes for IO functions (#1733)
Browse files Browse the repository at this point in the history
While sorting out lifetime problems to make #1731 landable, I noticed
JsonInOutFuncs is completely replaceable with two reexports. So I
replaced it. This also made the changes to sort out the rest of the
lifetimes obvious, and now we return CString always from these
functions.
  • Loading branch information
workingjubilee authored Jun 7, 2024
1 parent a0c3de6 commit 53ed4da
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 50 deletions.
41 changes: 14 additions & 27 deletions pgrx-macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ fn impl_postgres_type(ast: DeriveInput) -> syn::Result<proc_macro2::TokenStream>

let lifetime = match has_lifetimes {
Some(lifetime) => quote! {#lifetime},
None => quote! {'static},
None => quote! {'_},
};

// all #[derive(PostgresType)] need to implement that trait
Expand Down Expand Up @@ -868,42 +868,29 @@ fn impl_postgres_type(ast: DeriveInput) -> syn::Result<proc_macro2::TokenStream>
// and if we don't have custom inout/funcs, we use the JsonInOutFuncs trait
// which implements _in and _out #[pg_extern] functions that just return the type itself
if args.contains(&PostgresTypeAttribute::Default) {
let inout_generics = if has_lifetimes.is_some() {
quote! {#generics}
} else {
quote! {<'_>}
};

stream.extend(quote! {
impl #generics ::pgrx::inoutfuncs::JsonInOutFuncs #inout_generics for #name #generics {}

#[doc(hidden)]
#[::pgrx::pgrx_macros::pg_extern(immutable,parallel_safe)]
pub fn #funcname_in #generics(input: Option<&#lifetime ::core::ffi::CStr>) -> Option<#name #generics> {
input.map_or_else(|| {
for m in <#name as ::pgrx::inoutfuncs::JsonInOutFuncs>::NULL_ERROR_MESSAGE {
::pgrx::pg_sys::error!("{m}");
}
None
}, |i| Some(<#name as ::pgrx::inoutfuncs::JsonInOutFuncs>::input(i)))
use ::pgrx::inoutfuncs::json_from_slice;
input.map(|cstr| json_from_slice(cstr.to_bytes()).ok()).flatten()
}

#[doc(hidden)]
#[::pgrx::pgrx_macros::pg_extern (immutable,parallel_safe)]
pub fn #funcname_out #generics(input: #name #generics) -> &'static ::core::ffi::CStr {
let mut buffer = ::pgrx::stringinfo::StringInfo::new();
::pgrx::inoutfuncs::JsonInOutFuncs::output(&input, &mut buffer);
// SAFETY: We just constructed this StringInfo ourselves
unsafe { buffer.leak_cstr() }
pub fn #funcname_out #generics(input: #name #generics) -> ::pgrx::ffi::CString {
use ::pgrx::inoutfuncs::json_to_vec;
let mut bytes = json_to_vec(&input).unwrap();
bytes.push(0); // terminate
::pgrx::ffi::CString::from_vec_with_nul(bytes).unwrap()
}

});
} else if args.contains(&PostgresTypeAttribute::InOutFuncs) {
// otherwise if it's InOutFuncs our _in/_out functions use an owned type instance
stream.extend(quote! {
#[doc(hidden)]
#[::pgrx::pgrx_macros::pg_extern(immutable,parallel_safe)]
pub fn #funcname_in #generics(input: Option<&#lifetime ::core::ffi::CStr>) -> Option<#name #generics> {
pub fn #funcname_in #generics(input: Option<&::core::ffi::CStr>) -> Option<#name #generics> {
input.map_or_else(|| {
for m in <#name as ::pgrx::inoutfuncs::InOutFuncs>::NULL_ERROR_MESSAGE {
::pgrx::pg_sys::error!("{m}");
Expand All @@ -914,19 +901,19 @@ fn impl_postgres_type(ast: DeriveInput) -> syn::Result<proc_macro2::TokenStream>

#[doc(hidden)]
#[::pgrx::pgrx_macros::pg_extern(immutable,parallel_safe)]
pub fn #funcname_out #generics(input: #name #generics) -> &'static ::core::ffi::CStr {
pub fn #funcname_out #generics(input: #name #generics) -> ::pgrx::ffi::CString {
let mut buffer = ::pgrx::stringinfo::StringInfo::new();
::pgrx::inoutfuncs::InOutFuncs::output(&input, &mut buffer);
// SAFETY: We just constructed this StringInfo ourselves
unsafe { buffer.leak_cstr() }
unsafe { buffer.leak_cstr().to_owned() }
}
});
} else if args.contains(&PostgresTypeAttribute::PgVarlenaInOutFuncs) {
// otherwise if it's PgVarlenaInOutFuncs our _in/_out functions use a PgVarlena
stream.extend(quote! {
#[doc(hidden)]
#[::pgrx::pgrx_macros::pg_extern(immutable,parallel_safe)]
pub fn #funcname_in #generics(input: Option<&#lifetime ::core::ffi::CStr>) -> Option<::pgrx::datum::PgVarlena<#name #generics>> {
pub fn #funcname_in #generics(input: Option<&::core::ffi::CStr>) -> Option<::pgrx::datum::PgVarlena<#name #generics>> {
input.map_or_else(|| {
for m in <#name as ::pgrx::inoutfuncs::PgVarlenaInOutFuncs>::NULL_ERROR_MESSAGE {
::pgrx::pg_sys::error!("{m}");
Expand All @@ -937,11 +924,11 @@ fn impl_postgres_type(ast: DeriveInput) -> syn::Result<proc_macro2::TokenStream>

#[doc(hidden)]
#[::pgrx::pgrx_macros::pg_extern(immutable,parallel_safe)]
pub fn #funcname_out #generics(input: ::pgrx::datum::PgVarlena<#name #generics>) -> &'static ::core::ffi::CStr {
pub fn #funcname_out #generics(input: ::pgrx::datum::PgVarlena<#name #generics>) -> ::pgrx::ffi::CString {
let mut buffer = ::pgrx::stringinfo::StringInfo::new();
::pgrx::inoutfuncs::PgVarlenaInOutFuncs::output(&*input, &mut buffer);
// SAFETY: We just constructed this StringInfo ourselves
unsafe { buffer.leak_cstr() }
unsafe { buffer.leak_cstr().to_owned() }
}
});
}
Expand Down
1 change: 1 addition & 0 deletions pgrx/src/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@
//LICENSE
//LICENSE Use of this source code is governed by the MIT license that can be found in the LICENSE file.
//! Reexport FFI definitions here.
pub use alloc::ffi::CString;
pub use libc::c_char;
24 changes: 2 additions & 22 deletions pgrx/src/inoutfuncs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
//! and `serde_cbor` to serialize internally as a `varlena *` for storage on disk.
use crate::*;
#[doc(hidden)]
pub use serde_json::{from_slice as json_from_slice, to_vec as json_to_vec};

/// `#[derive(Copy, Clone, PostgresType)]` types need to implement this trait to provide the text
/// input/output functions for that type
Expand Down Expand Up @@ -50,25 +52,3 @@ pub trait InOutFuncs {
/// error message should be generated?
const NULL_ERROR_MESSAGE: Option<&'static str> = None;
}

/// Automatically implemented for `#[derive(Serialize, Deserialize, PostgresType)]` types that do
/// **not** also have the `#[inoutfuncs]` attribute macro
pub trait JsonInOutFuncs<'de>: serde::de::Deserialize<'de> + serde::ser::Serialize {
/// Uses `serde_json` to deserialize the input, which is assumed to be JSON
fn input(input: &'de core::ffi::CStr) -> Self {
serde_json::from_str(input.to_str().expect("text input is not valid UTF8"))
.expect("failed to deserialize json")
}

/// Users `serde_json` to serialize `Self` into JSON
fn output(&self, buffer: &mut StringInfo)
where
Self: serde::ser::Serialize,
{
serde_json::to_writer(buffer, self).expect("failed to serialize to json")
}

/// If PostgreSQL calls the conversion function with NULL as an argument, what
/// error message should be generated?
const NULL_ERROR_MESSAGE: Option<&'static str> = None;
}
2 changes: 1 addition & 1 deletion pgrx/src/prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub use crate::datum::{
Numeric, PgVarlena, PostgresType, Range, RangeBound, RangeSubType, Time, TimeWithTimeZone,
Timestamp, TimestampWithTimeZone, VariadicArray,
};
pub use crate::inoutfuncs::{InOutFuncs, JsonInOutFuncs, PgVarlenaInOutFuncs};
pub use crate::inoutfuncs::{InOutFuncs, PgVarlenaInOutFuncs};

// Trigger support
pub use crate::trigger_support::{
Expand Down

0 comments on commit 53ed4da

Please sign in to comment.