From 09d507e89318a69ab8ee3a210bbe76056544709c Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 27 Oct 2023 17:10:47 -0700 Subject: [PATCH 1/6] Mark actually-unsafe pub fn as unsafe These fn are unsound to expose as safe functions. --- pgrx/src/aggregate.rs | 2 +- pgrx/src/htup.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pgrx/src/aggregate.rs b/pgrx/src/aggregate.rs index 458b2740a..1cdd4d810 100644 --- a/pgrx/src/aggregate.rs +++ b/pgrx/src/aggregate.rs @@ -433,7 +433,7 @@ where } #[inline(always)] - fn in_memory_context< + unsafe fn in_memory_context< R, F: FnOnce(&mut PgMemoryContexts) -> R + std::panic::UnwindSafe + std::panic::RefUnwindSafe, >( diff --git a/pgrx/src/htup.rs b/pgrx/src/htup.rs index 6c87cada7..fea1cdcb9 100644 --- a/pgrx/src/htup.rs +++ b/pgrx/src/htup.rs @@ -46,7 +46,7 @@ pub fn heap_tuple_header_get_datum_length(htup_header: pg_sys::HeapTupleHeader) /// convert a HeapTupleHeader to a Datum. #[inline] -pub fn heap_tuple_get_datum(heap_tuple: pg_sys::HeapTuple) -> pg_sys::Datum { +pub unsafe fn heap_tuple_get_datum(heap_tuple: pg_sys::HeapTuple) -> pg_sys::Datum { unsafe { pg_sys::HeapTupleHeaderGetDatum((*heap_tuple).t_data) } } From c1dfec60b910cb7f0e7b61e4ecb222b31bfc6e51 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 27 Oct 2023 17:30:16 -0700 Subject: [PATCH 2/6] Make unsafe assertions in aggregate codegen These may be completely unjustified, but they probably hold. --- pgrx-sql-entity-graph/src/aggregate/mod.rs | 90 +++++++++++++--------- 1 file changed, 54 insertions(+), 36 deletions(-) diff --git a/pgrx-sql-entity-graph/src/aggregate/mod.rs b/pgrx-sql-entity-graph/src/aggregate/mod.rs index b446faf39..93566e44e 100644 --- a/pgrx-sql-entity-graph/src/aggregate/mod.rs +++ b/pgrx-sql-entity-graph/src/aggregate/mod.rs @@ -299,10 +299,12 @@ impl PgAggregate { #[allow(non_snake_case, clippy::too_many_arguments)] #pg_extern_attr fn #fn_name(this: #type_state_without_self, #(#args_with_names),*, fcinfo: ::pgrx::pg_sys::FunctionCallInfo) -> #type_state_without_self { - <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( - fcinfo, - move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::state(this, (#(#arg_names),*), fcinfo) - ) + unsafe { + <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( + fcinfo, + move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::state(this, (#(#arg_names),*), fcinfo) + ) + } } }); fn_name @@ -322,10 +324,12 @@ impl PgAggregate { #[allow(non_snake_case, clippy::too_many_arguments)] #pg_extern_attr fn #fn_name(this: #type_state_without_self, v: #type_state_without_self, fcinfo: ::pgrx::pg_sys::FunctionCallInfo) -> #type_state_without_self { - <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( - fcinfo, - move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::combine(this, v, fcinfo) - ) + unsafe { + <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( + fcinfo, + move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::combine(this, v, fcinfo) + ) + } } }); Some(fn_name) @@ -351,10 +355,12 @@ impl PgAggregate { #[allow(non_snake_case, clippy::too_many_arguments)] #pg_extern_attr fn #fn_name(this: #type_state_without_self, #(#direct_args_with_names),*, fcinfo: ::pgrx::pg_sys::FunctionCallInfo) -> #type_finalize { - <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( - fcinfo, - move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::finalize(this, (#(#direct_arg_names),*), fcinfo) - ) + unsafe { + <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( + fcinfo, + move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::finalize(this, (#(#direct_arg_names),*), fcinfo) + ) + } } }); } else { @@ -362,10 +368,12 @@ impl PgAggregate { #[allow(non_snake_case, clippy::too_many_arguments)] #pg_extern_attr fn #fn_name(this: #type_state_without_self, fcinfo: ::pgrx::pg_sys::FunctionCallInfo) -> #type_finalize { - <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( - fcinfo, - move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::finalize(this, (), fcinfo) - ) + unsafe { + <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( + fcinfo, + move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::finalize(this, (), fcinfo) + ) + } } }); }; @@ -388,10 +396,12 @@ impl PgAggregate { #[allow(non_snake_case, clippy::too_many_arguments)] #pg_extern_attr fn #fn_name(this: #type_state_without_self, fcinfo: ::pgrx::pg_sys::FunctionCallInfo) -> Vec { - <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( - fcinfo, - move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::serial(this, fcinfo) - ) + unsafe { + <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( + fcinfo, + move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::serial(this, fcinfo) + ) + } } }); Some(fn_name) @@ -415,10 +425,12 @@ impl PgAggregate { #[allow(non_snake_case, clippy::too_many_arguments)] #pg_extern_attr fn #fn_name(this: #type_state_without_self, buf: Vec, internal: ::pgrx::pgbox::PgBox<#type_state_without_self>, fcinfo: ::pgrx::pg_sys::FunctionCallInfo) -> ::pgrx::pgbox::PgBox<#type_state_without_self> { - <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( - fcinfo, - move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::deserial(this, buf, internal, fcinfo) - ) + unsafe { + <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( + fcinfo, + move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::deserial(this, buf, internal, fcinfo) + ) + } } }); Some(fn_name) @@ -447,10 +459,12 @@ impl PgAggregate { #(#args_with_names),*, fcinfo: ::pgrx::pg_sys::FunctionCallInfo, ) -> #type_moving_state { - <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( - fcinfo, - move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::moving_state(mstate, (#(#arg_names),*), fcinfo) - ) + unsafe { + <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( + fcinfo, + move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::moving_state(mstate, (#(#arg_names),*), fcinfo) + ) + } } }); Some(fn_name) @@ -483,10 +497,12 @@ impl PgAggregate { #(#args_with_names),*, fcinfo: ::pgrx::pg_sys::FunctionCallInfo, ) -> #type_moving_state { - <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( - fcinfo, - move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::moving_state_inverse(mstate, (#(#arg_names),*), fcinfo) - ) + unsafe { + <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( + fcinfo, + move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::moving_state_inverse(mstate, (#(#arg_names),*), fcinfo) + ) + } } }); Some(fn_name) @@ -517,10 +533,12 @@ impl PgAggregate { #[allow(non_snake_case, clippy::too_many_arguments)] #pg_extern_attr fn #fn_name(mstate: #type_moving_state, #(#direct_args_with_names),* #maybe_comma fcinfo: ::pgrx::pg_sys::FunctionCallInfo) -> #type_finalize { - <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( - fcinfo, - move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::moving_finalize(mstate, (#(#direct_arg_names),*), fcinfo) - ) + unsafe { + <#target_path as ::pgrx::aggregate::Aggregate>::in_memory_context( + fcinfo, + move |_context| <#target_path as ::pgrx::aggregate::Aggregate>::moving_finalize(mstate, (#(#direct_arg_names),*), fcinfo) + ) + } } }); Some(fn_name) From 1cf6fb542edd83bab465f36d130def49a90430bb Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 31 Oct 2023 14:46:03 -0700 Subject: [PATCH 3/6] Add clippy with -Awarnings to CI --- .github/workflows/tests.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index cddc12756..e22e77540 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -28,7 +28,7 @@ jobs: strategy: fail-fast: false matrix: - version: ["postgres-12", "postgres-13", "postgres-14", "postgres-15", "postgres-16"] + version: ["postgres-11", "postgres-12", "postgres-13", "postgres-14", "postgres-15", "postgres-16"] steps: - uses: actions/checkout@v3 @@ -351,6 +351,9 @@ jobs: - name: Run 'cargo pgrx init' for ${{ matrix.version }} run: cargo pgrx init --pg$PG_VER download + - name: Clippy -Awarnings + run: cargo clippy -- -Awarnings + - name: create new sample extension run: cd /tmp/ && cargo pgrx new sample From d49858be3bf072c47223ffe1b755053ece563959 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 31 Oct 2023 15:06:23 -0700 Subject: [PATCH 4/6] Also failfast again --- .github/workflows/tests.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e22e77540..afb80b04b 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -26,9 +26,8 @@ jobs: SCCACHE_DIR: /home/runner/.cache/sccache strategy: - fail-fast: false matrix: - version: ["postgres-11", "postgres-12", "postgres-13", "postgres-14", "postgres-15", "postgres-16"] + version: ["postgres-12", "postgres-13", "postgres-14", "postgres-15", "postgres-16"] steps: - uses: actions/checkout@v3 From 5e3d6a12eb34f5ddbe67fd151f51a7b9d7d108bb Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 31 Oct 2023 15:28:18 -0700 Subject: [PATCH 5/6] Hack in clippy test with version --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index afb80b04b..9473adf3f 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -351,7 +351,7 @@ jobs: run: cargo pgrx init --pg$PG_VER download - name: Clippy -Awarnings - run: cargo clippy -- -Awarnings + run: cargo clippy -p pgrx --features pg$PG_VER -- -Awarnings - name: create new sample extension run: cd /tmp/ && cargo pgrx new sample From 94943cac3dd510fac36ff9ae10ca31221846240e Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 31 Oct 2023 15:39:01 -0700 Subject: [PATCH 6/6] Provide fixed Ord impl for Numeric --- pgrx/src/datum/numeric_support/cmp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pgrx/src/datum/numeric_support/cmp.rs b/pgrx/src/datum/numeric_support/cmp.rs index e4cb81d25..b9e3fa42b 100644 --- a/pgrx/src/datum/numeric_support/cmp.rs +++ b/pgrx/src/datum/numeric_support/cmp.rs @@ -102,7 +102,7 @@ impl Eq for Numeric {} impl PartialOrd for Numeric { #[inline] fn partial_cmp(&self, other: &Self) -> Option { - self.as_anynumeric().partial_cmp(other.as_anynumeric()) + Some(self.cmp(other)) } #[inline]