add new catalog and cache APIs #1545
Conversation
usamoi
force-pushed
the
catalog
branch
8 times, most recently
from
8b6b4f2 to
760420f
Compare
|
Failed compile-fail tests make me really confused... 
|
usamoi
force-pushed
the
catalog
branch
3 times, most recently
from
e1e20e3 to
c0cb502
Compare
|
Added documents copied from PostgreSQL website. |
|
Ah, most of them were copied, huh? Hm. Will have to think a little about that. |
There was a problem hiding this comment.
I can follow the macro expansion patterns now, but I'm somewhat uneasy about the code and its durability in the future. I would prefer that this came with more ways of enforcing the expansion was correct, and more explanation as to why the unsafe code is correct, so that it is easier to evaluate changes of it in terms of soundness. Especially if e.g. Postgres changes the size of a value and invalidates a safety assumption we are tacitly relying on.
pgrx/src/pg_catalog.rs
Outdated
| #[doc = concat!("Safe wrapper for pg_catalog.", stringify!($table), ".")] | ||
| $(#[$table_meta])* | ||
| pub struct [<$table:camel>]<'a> { | ||
| inner: &'a pgrx::pg_sys::HeapTupleData, |
There was a problem hiding this comment.
Normally I avoid holding references to variably-sized structs due to UCG#256 still being unresolved. I think it doesn't matter in this case.
pgrx/src/pg_catalog.rs
Outdated
| unsafe impl<T> GetStruct<T> for T { | ||
| unsafe fn get_struct(raw: *const T) -> Self { | ||
| unsafe { raw.read() } | ||
| } | ||
| } | ||
|
|
||
| unsafe impl GetStruct<pgrx::pg_sys::nameData> for &CStr { | ||
| unsafe fn get_struct(raw: *const pgrx::pg_sys::nameData) -> Self { | ||
| unsafe { CStr::from_ptr(raw.cast::<c_char>()) } | ||
| } | ||
| } | ||
|
|
||
| unsafe impl GetStruct<pgrx::pg_sys::int2vector> for &[i16] { | ||
| unsafe fn get_struct(raw: *const pgrx::pg_sys::int2vector) -> Self { | ||
| unsafe { (*raw).values.as_slice((*raw).dim1 as usize) } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm guessing the reason we need this is because sometimes we want to read the data, sometimes we want to get a CStr, and sometimes we need a slice, huh? But what about the other types that terminate in VLAs?
There was a problem hiding this comment.
All members in FromData_* are fixed-sized. There are only 3 exceptions: oidvector, int2vector and bytea. If there is oidvector, int2vector or bytea (only in pg_largeobject) in FromData_*, they must be the last struct member. Someone who needs bytea or there are more variable-sized types in future PostgreSQL versions could add it by themselves.
nameData is fixed-sized 64-byte array in fact. It's guaranteed that it's nul-padded so we can get CStr from it and we expose &CStr to users since it's more friendly.
|
The type of a pg_catalog column is unlikely to change. But if it changes,
If we want to check the second condition, we can get oid of the attribute in run-time (or test codes) and compare it with what we record in macro-call statements. Will the check make you more confident? |
|
@usamoi Oh, I'm less concerned about runtime types, and more concerned about a later code change by someone else making something subtly wrong that nonetheless the macros allow, and it being hard to tell. |
|
Unsafe code is mostly moved out of There are still several unsafe blocks like |
|
@workingjubilee Could you give me some advice about this patch? If it could not be merged, I feel it's better to move code into a downstream package. |
| } | ||
| } | ||
|
|
||
| define_catalog! { |
There was a problem hiding this comment.
Just picking one of these at random...
You hand-wrote all of these define_catalog!() declarations? I don't see that as sustainable. Is there any way we can generate this, via build.rs, from the Postgres sources?
Even if that meant we'd need our own equivalent of genbki.pl (in Rust, obviously), that will be infinitely better than trying to manually keep these up-to-date. Postgres does change catalogs from time to time.
There was a problem hiding this comment.
All declarations are hand-written. If a new column is added in a newer version of PostgreSQL, who uses it could add it again, so I thought it's not a problem.
It looks that this process could be done by parsing catalog/pg_*.h and catalogs.sgml. I will try it later.
There was a problem hiding this comment.
who uses it could add it again
The problem with that is it's not guaranteed anyone would notice, which might lead to miscompilations or runtime UB or who-knows-what.
In build.rs, we ultimately have the struct definitions as provided by bindgen. Maybe those can be traversed to generate the remaining code?
Ultimately, this would get us 100% coverage of all the catalogs.
There was a problem hiding this comment.
That does seem like it would be best, although it might be a bit toilsome to implement the first time and occasionally to keep up to date.
Signed-off-by: usamoi <[email protected]>
Signed-off-by: usamoi <[email protected]>
Signed-off-by: usamoi <[email protected]>
Provide a safe wrapper of PostgreSQL catalog and cache.