Low-level interop for PG arrays #636
Conversation
There was a problem hiding this comment.
Do the helper functions have to live in pgx-pg-sys? Because they should be purely additive to the sys crate, I would somewhat prefer they were exposed through a mod in the main pgx crate, so as to make it easier to thread them through the other code in pgx without violating coherence.
I also would like it if all new unsafe code that lands in PGX to document its public /// # Safety requirements as precisely as possible, and for every invocation of unsafe to come with // SAFETY: annotations, and I would prefer it to come with #[deny(unsafe_op_in_unsafe_fn)] on the containing module so as to require both of those even inside an unsafe fn.
It is the last one, the extreme safety requirements, that is completely impractical to implement throughout pgx-pg-sys. Note that the comments I desire can be very terse, they do not have to be elaborate, they just have to make clear all the little details like "this does a field read".
|
The burden of assuring memory safety must end the moment code leaves an e.g. It is fine if The direct implication is that safe code is allowed to "shit the bed", from the perspective of ...Directly, mind. Unfortunately running |
This offers safe accessors to the underlying ArrayType's fields, without imposing any more burden on the data in question.
This also ports over ARR_DIMS(ArrayType*), and ARR_NELEMS, which wraps ArrayGetNItems(ndim, *dims)
Includes making it non-pub.
Includes a general usage note on lengths.
pgx/src/array.rs
Outdated
| // for expected behavior, see: | ||
| // postgres/src/include/utils/array.h | ||
| // #define ARR_DIMS |
There was a problem hiding this comment.
Should this path be visible in the public documentation? I suppose they can click through to see the source, but the public docs will support links.
There was a problem hiding this comment.
Yeah, actually, directly linking is a good idea. And that way we could also link to the doxygen.
There was a problem hiding this comment.
Doxygen is too unstable, will point to master and lines might change. Instead used gitweb links.
There was a problem hiding this comment.
From a pgx-maintainers perspective, how much of a maintenance nightmare is it linking to the doxygen pages? How often would that change?
There was a problem hiding this comment.
Potentially every time someone lands a commit into postgresql/master.
Includes links that are permanent to some version of Postgres.
workingjubilee
changed the title
pgx/src/array.rs
Outdated
| /** | ||
| The ability to rewrite the dimensions slice. | ||
|
|
||
| You almost certainly do not actually want to call this, | ||
| unless you intentionally stored the actually intended ndim and wrote 0 instead. | ||
| Returns a triple tuple of | ||
| * a mutable reference to the underlying ArrayType's ndim field | ||
| * a pointer to the first c_int of the dimensions slice | ||
| * a mutable reference to RawArray's len field | ||
|
|
||
| Write to them in order. | ||
| */ | ||
| pub unsafe fn dims_mut(&mut self) -> (&mut libc::c_int, NonNull<libc::c_int>, &mut usize) { | ||
| // SAFETY: Validity asserted on construction, origin ptr is non-null. | ||
| let dims_ptr = unsafe { NonNull::new_unchecked(pgx_ARR_DIMS(self.ptr.as_ptr())) }; | ||
| let len = &mut self.len; | ||
|
|
||
| // SAFETY: Validity asserted on construction. Just reborrowing a subfield. | ||
| (unsafe { &mut self.ptr.as_mut().ndim }, dims_ptr, len) | ||
| } |
There was a problem hiding this comment.
You ain't gonna need it, tho'.
There was a problem hiding this comment.
Okaaaay it was just fun to write.
These PG function shims will allow for future work to improve the
pgx::Array<T>type, as well as manual access to ArrayType data in the meantime.