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

Address pg16 rename of *_tree_walker fn #1596

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pgrx-pg-sys/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -741,7 +741,7 @@ fn add_blocklists(bind: bindgen::Builder) -> bindgen::Builder {
bind.blocklist_type("Datum") // manually wrapping datum for correctness
.blocklist_type("Oid") // "Oid" is not just any u32
.blocklist_function("varsize_any") // pgrx converts the VARSIZE_ANY macro, so we don't want to also have this function, which is in heaptuple.c
.blocklist_function("(?:query|expression)_tree_walker")
.blocklist_function("(?:raw_)?(?:query|expression)_tree_walker")
.blocklist_function(".*(?:set|long)jmp")
.blocklist_function("pg_re_throw")
.blocklist_function("err(start|code|msg|detail|context_msg|hint|finish)")
Expand Down
64 changes: 52 additions & 12 deletions pgrx-pg-sys/src/port.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use crate as pg_sys;

use memoffset::*;
use pgrx_macros::pg_guard;
use std::str::FromStr;

/// this comes from `postgres_ext.h`
Expand Down Expand Up @@ -229,29 +228,70 @@ pub unsafe fn heap_tuple_get_struct<T>(htup: super::HeapTuple) -> *mut T {
}
}

#[pg_guard]
// All of this weird code is in response to Postgres having a relatively cavalier attitude about types:
// - https://github.com/postgres/postgres/commit/1c27d16e6e5c1f463bbe1e9ece88dda811235165
//
// As a result, we redeclare their functions with the arguments they should have on earlier Postgres
// and we route people to the old symbols they were using before on later ones.
#[cfg(any(feature = "pg12", feature = "pg13", feature = "pg14", feature = "pg15"))]
#[::pgrx_macros::pg_guard]
extern "C" {
pub fn query_tree_walker(
query: *mut super::Query,
walker: ::std::option::Option<
unsafe extern "C" fn(*mut super::Node, *mut ::std::os::raw::c_void) -> bool,
walker: ::core::option::Option<
unsafe extern "C" fn(*mut super::Node, *mut ::core::ffi::c_void) -> bool,
>,
context: *mut ::std::os::raw::c_void,
flags: ::std::os::raw::c_int,
context: *mut ::core::ffi::c_void,
flags: ::core::ffi::c_int,
Comment on lines -232 to +245
Copy link
Member Author

@workingjubilee workingjubilee Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lack of conditions in the original version made this potentially unsound when called on Postgres 16, since then you just... jump to nothing, I guess? Hope the dynamic loader errors properly! "You mean the linker." We defer symbol resolution to runtime on some platforms!

Honestly, redeclaring arguments like this is also pretty fucking UB, but I guess everything sucks!

) -> bool;
}

#[pg_guard]
extern "C" {
pub fn expression_tree_walker(
node: *mut super::Node,
walker: ::std::option::Option<
unsafe extern "C" fn(*mut super::Node, *mut ::std::os::raw::c_void) -> bool,
walker: ::core::option::Option<
unsafe extern "C" fn(*mut super::Node, *mut ::core::ffi::c_void) -> bool,
>,
context: *mut ::core::ffi::c_void,
) -> bool;

pub fn raw_expression_tree_walker(
node: *mut super::Node,
walker: ::core::option::Option<
unsafe extern "C" fn(*mut super::Node, *mut ::core::ffi::c_void) -> bool,
>,
context: *mut ::std::os::raw::c_void,
context: *mut ::core::ffi::c_void,
) -> bool;
}

#[cfg(feature = "pg16")]
pub unsafe fn query_tree_walker(
query: *mut super::Query,
walker: ::core::option::Option<
unsafe extern "C" fn(*mut super::Node, *mut ::core::ffi::c_void) -> bool,
>,
context: *mut ::core::ffi::c_void,
flags: ::core::ffi::c_int,
) -> bool {
crate::query_tree_walker_impl(query, walker, context, flags)
}

#[cfg(feature = "pg16")]
pub unsafe fn expression_tree_walker(
arg_node: *mut crate::Node,
arg_walker: Option<unsafe extern "C" fn(*mut crate::Node, *mut ::core::ffi::c_void) -> bool>,
arg_context: *mut ::core::ffi::c_void,
) -> bool {
crate::expression_tree_walker_impl(arg_node, arg_walker, arg_context)
}

#[cfg(feature = "pg16")]
pub unsafe fn raw_expression_tree_walker(
arg_node: *mut crate::Node,
arg_walker: Option<unsafe extern "C" fn(*mut crate::Node, *mut ::core::ffi::c_void) -> bool>,
arg_context: *mut ::core::ffi::c_void,
) -> bool {
crate::raw_expression_tree_walker_impl(arg_node, arg_walker, arg_context)
}

#[inline(always)]
pub unsafe fn MemoryContextSwitchTo(context: crate::MemoryContext) -> crate::MemoryContext {
let old = crate::CurrentMemoryContext;
Expand Down
40 changes: 8 additions & 32 deletions pgrx-tests/src/tests/pg_try_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,48 +7,24 @@
//LICENSE All rights reserved.
//LICENSE
//LICENSE Use of this source code is governed by the MIT license that can be found in the LICENSE file.
use core::ptr;
use pgrx::prelude::*;

// if our Postgres ERROR and Rust panic!() handling is incorrect, this little bit of useless code
// will crash postgres. If things are correct it'll simply raise an ERROR saying "panic in walker".
#[pg_extern]
fn crash() {
#[cfg(feature = "cshim")]
{
use pgrx::PgList;
let mut node = PgList::<pg_sys::Node>::new();
node.push(PgList::<pg_sys::Node>::new().into_pg() as *mut pg_sys::Node);

#[cfg(any(feature = "pg12", feature = "pg13", feature = "pg14", feature = "pg15"))]
unsafe {
pg_sys::raw_expression_tree_walker(
node.into_pg() as *mut pg_sys::Node,
Some(walker),
std::ptr::null_mut(),
);
}

#[cfg(feature = "pg16")]
error!("panic in walker");
}
use pgrx::{list::List, memcx};
memcx::current_context(|current| unsafe {
let mut list = List::downcast_ptr_in_memcx(ptr::null_mut(), current).unwrap();
list.unstable_push_in_context(ptr::null_mut(), current);

#[cfg(not(feature = "cshim"))]
{
walker();
}
}

#[cfg(not(feature = "cshim"))]
fn walker() -> bool {
panic!("panic in walker");
pg_sys::raw_expression_tree_walker(list.as_mut_ptr().cast(), Some(walker), ptr::null_mut());
});
}

#[cfg(all(
feature = "cshim",
any(feature = "pg12", feature = "pg13", feature = "pg14", feature = "pg15")
))]
#[pg_guard]
extern "C" fn walker() -> bool {
extern "C" fn walker(_node: *mut pg_sys::Node, _void: *mut ::core::ffi::c_void) -> bool {
panic!("panic in walker");
}

Expand Down