Array-walking is aligned #1191

workingjubilee · 2023-07-05T16:11:48Z

The alignment-handling code has been correct for some time.

Remove the questioning assert, extract the align-up pattern into a function on layout::Align, and precompute the fixed-size offset to use for fixed-size elements in arrays.

workingjubilee · 2023-07-05T16:12:20Z

This addresses one assert failure in #1185

eeeebbbbrrrr · 2023-07-05T16:19:27Z

This looks fine. I guess the problem here is that we were continuing to assert something that's actually not true, or otherwise properly handled by the follow-up changes after zero-copy-arrays first landed?

workingjubilee · 2023-07-05T16:22:34Z

Correct. I am not even sure if it was still unhandled by the end of the first PR, but somewhere after the Box<dyn ChaChaSlide> refactor, it was definitely not a concern.

Because the intervals are static, they can be calculated once.

The assumptions in this assert have long-since been corrected. Fixed-size array-walking is correct for all sizes and alignments.

workingjubilee · 2023-07-05T19:27:23Z

I need to add an assert elsewhere, I think?

workingjubilee · 2023-07-05T19:41:38Z

Added appropriate debug asserts for "YES this pointer is inbounds of the array!" These will be lighter weight than certain other notorious debug asserts we had.

workingjubilee · 2023-07-05T19:51:16Z

Oh lol, this debug assert is wrong.

pgrx/src/datum/array.rs

thomcc · 2023-07-05T21:21:21Z

pgrx/src/datum/array.rs

@@ -628,6 +606,10 @@ impl<'a, T: FromDatum> Iterator for ArrayIterator<'a, T> {
        let Some(is_null) = array.null_slice.get(*curr) else { return None };
        *curr += 1;

+        // Make sure a previous iter didn't push out of bounds!
+        if !is_null {
+            debug_assert!(*ptr < array.raw.end_ptr());


Note that the comparison is wrong if ptr wrapped around the address space and other such edge cases. Might not be worth giving a shit about, since handling that correctly is annoying.

Actually, I have some code in a non-work project that cares about that, so I tweaked it for pgrx (hopefully not breaking it in the process) which gives https://gist.github.com/thomcc/b2730e3b1a56c4d8cdebf29814ea2351. It can probably be simplified for our use though (we perhaps don't need the size param), and has not been tested after I tweaked it for pgrx. Actually, Something like this on RawArray is simpler but still sufficient.

#[inline] pub(crate) fn is_in_bounds(&self, p: *const u8) -> bool { let base = self.ptr.as_ptr(); let mem_size = unsafe { crate::varlena::varsize_any(base.cast()) }; // Note: This intentionally underflows if `p < base`. // If that happens, the wraparound will cause the result to // be greater than `mem_size` (we assume that the size postgres // gave us is reasonable, e.g. less than `isize::MAX`). (p as usize).wrapping_sub(base as usize) <= mem_size }

If you go that route, we have a couple other places where we have similar debug assertions comparing against end_ptr, might be worth fixing them too.

Can we land that in a separate PR? I probably want it but we should discuss it as its own beast because that raises questions about more of our codebase and this patch has already scope-creeped a bit.

Sure. We talked out of band (I don't think it raises other questions about most of the code base, just places where we might have broken address arithmetic)

workingjubilee · 2023-07-05T21:21:32Z

pgrx/src/datum/array.rs

@@ -450,7 +440,7 @@ mod casper {

    /// Array elements are [`pg_sys::varlena`] types, which are pass-by-reference
    pub(super) struct PassByVarlena {
-        pub(super) align: usize,
+        pub(super) align: Align,


Carrying a full usize is unnecessary here and in any case makes the result that much slightly-less typesafe.

pgrx/src/datum/array.rs

Co-authored-by: Thom Chovioloni <[email protected]>

* Precompute fixed-size array-walking Because the intervals are static, they can be calculated once. * Remove now-unnecessary assert The assumptions in this assert have long-since been corrected. Fixed-size array-walking is correct for all sizes and alignments. * Canonize Align::pad fn Remove code duplication and use more conventional forms. Co-authored-by: Thom Chovioloni <[email protected]>

@workingjubilee

This is the final release of v0.10.0. Thanks everyone for the beta testing, pull requests, issues, and patience. As always, install `cargo-pgrx` with `cargo install cargo-pgrx --locked` and update your extension Cargo.toml files to use the `0.10.0` pgrx dependencies. This release includes support for Postgres 16RC1. Support for the previous betas has been removed. As such, a fresh `cargo pgrx init` is required. ## What's Changed Since v0.10.0-beta.4 * Fix `GetMemoryChunkContext` port by @workingjubilee in #1273 * Better error messages when `pg_config` isn't found. by @eeeebbbbrrrr in #1271 * Make `PostgresHash` also need `Eq` by @workingjubilee in #1264 * Memoize git hash and extension metadata by @levkk in #1274 * move to pg16rc1 by @eeeebbbbrrrr in #1276 * Fix bgworker template up to 0.10.0-beta.4 by @workingjubilee in #1270 ## New Contributors * @levkk made their first contribution in #1274 **Changelog**: v0.10.0-beta.4...v0.10.0 --- v0.10.0's full set of changes throughout the entire beta period are: * Postgres 16beta1 Support by @eeeebbbbrrrr in #1169 * Support building against macOS universal binaries by @clowder in #1166 * list specific versions in feature gates by @eeeebbbbrrrr in #1175 * Fix bug with converting a `pg_sys::Datum` into a `pgrx::Date` by @eeeebbbbrrrr in #1177 * Fix Arrays with leading nulls by @eeeebbbbrrrr in #1180 * Disable hello_versioned_so test by @workingjubilee in #1192 * doc: fix link broken by @yihong0618 in #1181 * fcinfo: fix incorrect length set in unsafe code by @Sasasu in #1190 * update to pg16beta2 support by @eeeebbbbrrrr in #1188 * Array-walking is aligned by @workingjubilee in #1191 * Implement PGRXSharedMemory for Deque by @feikesteenbergen in #1170 * Include security labels header by @daamien in #1189 * Fixes macos-11 tests by @BradyBonnette in #1197 * Pgcentralfoundation updates again by @eeeebbbbrrrr in #1200 * Update version to 0.10.0-beta.0 by @eeeebbbbrrrr in #1201 * Testing help by @eeeebbbbrrrr in #1203 * Type testability cleanup by @eeeebbbbrrrr in #1204 * Try to smartly propagate fs errors by @workingjubilee in #1186 * Fix issue #1209 by @eeeebbbbrrrr in #1210 * Type roundtrip tests by @eeeebbbbrrrr in #1185 * Update version to 0.10.0-beta.1 by @eeeebbbbrrrr in #1213 * Add a workaround for the pg16/homebrew/icu4c situation by @thomcc in #1206 * Add security policy by @johnrballard in #1207 * `AnyNumeric` is no longer backed by Postgres-allocated memory by @eeeebbbbrrrr in #1216 * Modularize pgrx::spi by @workingjubilee in #1219 * Stop SpiClient soundness from regressing by @workingjubilee in #1214 * Add foreign table headers by @workingjubilee in #1226 * Modularize the interior of pgrx-pg-sys by @workingjubilee in #1227 * Initial valgrind support by @thomcc in #1218 * Add support for handling SIGINT and SIGCHLD from bgworker by @JelteF in #1229 * Ignores UI tests for MUSL environments by @BradyBonnette in #1235 * Add a env flag that can be set to skip `#[pg_test]`-generated tests. by @thomcc in #1239 * Fix issue #1076: Properly handle dependency graph of `Result<T, _>` by @eeeebbbbrrrr in #1241 * Cleanup the error when cargo-pgrx version doesn't match Cargo.toml by @eeeebbbbrrrr in #1240 * Add operator and cache related api by @VoVAllen in #1242 * Addresses cargo-pgrx error reporting by @BradyBonnette in #1238 * Update version to 0.10.0-beta.2 by @eeeebbbbrrrr in #1244 * Bump cargo-metadata and clap-cargo by @thomcc in #1246 * Derive Clone for Inet by @JelteF in #1251 * Correct docs for datetime `From` impls by @workingjubilee in #1253 * Only enable line tables for profile.dev by @thomcc in #1249 * Remove references to master branch by @thomcc in #1243 * Ensure bindgen gets all the `cppflags` it needs (on macOS, anyway) by @thomcc in #1247 * update for pg16beta3 support by @eeeebbbbrrrr in #1254 * Update version to 0.10.0-beta.3 by @eeeebbbbrrrr in #1255 * Add proptest support by @workingjubilee in #1258 * Misc reformatting and typo fixes by @workingjubilee in #1260 * spi: simplify (optimize?) Datum preparation by @vrmiguel in #1256 * Assume commutation when deriving PostgresEq by @workingjubilee in #1261 * Demand Ord for PostgresOrd by @workingjubilee in #1262 * Fix pgrx install causing postgresql coredump by @Sasasu in #1263 * Update version to 0.10.0-beta.4 by @workingjubilee in #1267 ## New Contributors * @clowder made their first contribution in #1166 * @yihong0618 made their first contribution in #1181 * @Sasasu made their first contribution in #1190 * @daamien made their first contribution in #1189 * @johnrballard made their first contribution in #1207 * @VoVAllen made their first contribution in #1242 * @vrmiguel made their first contribution in #1256 **Full Changelog**: v0.9.8...v0.10.0

workingjubilee requested review from thomcc and eeeebbbbrrrr July 5, 2023 16:11

eeeebbbbrrrr approved these changes Jul 5, 2023

View reviewed changes

workingjubilee added 2 commits July 5, 2023 10:38

Precompute fixed-size array-walking

ea287a8

Because the intervals are static, they can be calculated once.

Remove now-unnecessary assert

91122a7

The assumptions in this assert have long-since been corrected. Fixed-size array-walking is correct for all sizes and alignments.

workingjubilee force-pushed the fixedsize-array-walking branch from 131abc7 to 91122a7 Compare July 5, 2023 17:38

workingjubilee commented Jul 5, 2023

View reviewed changes

pgrx/src/datum/array.rs Outdated Show resolved Hide resolved

thomcc reviewed Jul 5, 2023

View reviewed changes

pgrx/src/datum/array.rs Outdated Show resolved Hide resolved

pgrx/src/datum/array.rs Outdated Show resolved Hide resolved

workingjubilee force-pushed the fixedsize-array-walking branch 2 times, most recently from 5f2a248 to 8a5e5a1 Compare July 5, 2023 20:57

workingjubilee changed the title ~~Fixed-size array walking~~ Array-walking is aligned Jul 5, 2023

thomcc reviewed Jul 5, 2023

View reviewed changes

workingjubilee commented Jul 5, 2023

View reviewed changes

thomcc reviewed Jul 5, 2023

View reviewed changes

pgrx/src/datum/array.rs Outdated Show resolved Hide resolved

Canonize Align::pad fn

543e8b1

Co-authored-by: Thom Chovioloni <[email protected]>

workingjubilee force-pushed the fixedsize-array-walking branch from 2950c60 to 543e8b1 Compare July 5, 2023 21:42

thomcc approved these changes Jul 5, 2023

View reviewed changes

workingjubilee merged commit b84eee1 into pgcentralfoundation:develop Jul 6, 2023

workingjubilee deleted the fixedsize-array-walking branch July 6, 2023 01:24

eeeebbbbrrrr mentioned this pull request Jul 8, 2023

Update version to 0.10.0-beta.0 #1201

Merged

eeeebbbbrrrr mentioned this pull request Sep 5, 2023

Update version to 0.10.0 #1280

Merged

bayandin mentioned this pull request Sep 6, 2023

pgrx 0.10.0 bayandin/homebrew-tap#36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Array-walking is aligned #1191

Array-walking is aligned #1191

workingjubilee commented Jul 5, 2023 •

edited

Loading

workingjubilee commented Jul 5, 2023 •

edited

Loading

eeeebbbbrrrr commented Jul 5, 2023

workingjubilee commented Jul 5, 2023 •

edited

Loading

workingjubilee commented Jul 5, 2023 •

edited

Loading

workingjubilee commented Jul 5, 2023 •

edited

Loading

workingjubilee commented Jul 5, 2023

thomcc Jul 5, 2023 •

edited

Loading

thomcc Jul 5, 2023

workingjubilee Jul 5, 2023

thomcc Jul 5, 2023 •

edited

Loading

workingjubilee Jul 5, 2023

Array-walking is aligned #1191

Array-walking is aligned #1191

Conversation

workingjubilee commented Jul 5, 2023 • edited Loading

workingjubilee commented Jul 5, 2023 • edited Loading

eeeebbbbrrrr commented Jul 5, 2023

workingjubilee commented Jul 5, 2023 • edited Loading

workingjubilee commented Jul 5, 2023 • edited Loading

workingjubilee commented Jul 5, 2023 • edited Loading

workingjubilee commented Jul 5, 2023

thomcc Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

thomcc Jul 5, 2023

Choose a reason for hiding this comment

workingjubilee Jul 5, 2023

Choose a reason for hiding this comment

thomcc Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

workingjubilee Jul 5, 2023

Choose a reason for hiding this comment

workingjubilee commented Jul 5, 2023 •

edited

Loading

workingjubilee commented Jul 5, 2023 •

edited

Loading

workingjubilee commented Jul 5, 2023 •

edited

Loading

workingjubilee commented Jul 5, 2023 •

edited

Loading

workingjubilee commented Jul 5, 2023 •

edited

Loading

thomcc Jul 5, 2023 •

edited

Loading

thomcc Jul 5, 2023 •

edited

Loading