Use rust bindgen for reading/writing the PostgreSQL control file.

[hlinnaka]

No description provided.

[ericseppanen]

My findings so far on the use of transmute:

Since ControlFileData contains bools, transmute from a byte array is unsound (if the byte contains anything but 0 or 1, undefined behavior will result). If we expand our use of bindgen further, I expect we'll discover more ways to trigger dangerous things. So I'd prefer to avoid transmute on anything auto-generated: it's not obvious what types are in there, now or in the future.

I hoped there would be a way to attach #[derive(Serialize, Deserialize)] to the generated structs, but this doesn't seem possible right now. There are a few crates out there that also have this problem, and they seem to be using regex workarounds (example).

I also looked around at some of the "safe transmute" crates and RFC 2981. While there is a lot of potential there, I didn't find anything that's able to solve this problem now.

Some possible ways forward:
a) Continue with transmute until we can figure out how to do something better. Perhaps add a big warning comment, to remind us to fix this before it gets out of hand.
b) Go back to hand-written structs. We can replace the encode/decode functions with #[derive(Serialize, Deserialize)] if we like.
c) Build a procedural macro that can apply #[derive(Serialize, Deserialize)].
d) Run the bindgen output through a text regex to insert Serialize, Deserialize in the right place.

(d) seems crude, but it's so much less work than (c) that I think it's probably the right way forward, unless we expect to only have a few C structs imported with bindgen, then handwritten is probably fine.

[hlinnaka]

My findings so far on the use of transmute:

Since ControlFileData contains bools, transmute from a byte array is unsound (if the byte contains anything but 0 or 1, undefined behavior will result). If we expand our use of bindgen further, I expect we'll discover more ways to trigger dangerous things. So I'd prefer to avoid transmute on anything auto-generated: it's not obvious what types are in there, now or in the future.

Perhaps we could map Postgres bool to Rust's u8 instead of bool. Not sure how to tell bindgen to do that.

I hoped there would be a way to attach #[derive(Serialize, Deserialize)] to the generated structs, but this doesn't seem possible right now. There are a few crates out there that also have this problem, and they seem to be using regex workarounds (example).

Yeah, that would've been nice.

I also looked around at some of the "safe transmute" crates and RFC 2981. While there is a lot of potential there, I didn't find anything that's able to solve this problem now.

Some possible ways forward:
a) Continue with transmute until we can figure out how to do something better. Perhaps add a big warning comment, to remind us to fix this before it gets out of hand.
b) Go back to hand-written structs. We can replace the encode/decode functions with #[derive(Serialize, Deserialize)] if we like.
c) Build a procedural macro that can apply #[derive(Serialize, Deserialize)].
d) Run the bindgen output through a text regex to insert Serialize, Deserialize in the right place.

(d) seems crude, but it's so much less work than (c) that I think it's probably the right way forward, unless we expect to only have a few C structs imported with bindgen, then handwritten is probably fine.

I'm inclined to do a) for now, and revisit this and perhaps switch to d) if we need many more of these structs. If we're lucky, this gets addressed in bindgen in the meanwhile.

We have a few hand-written structs in the WAL decoding routines that mirror Postgres structs, but those are fairly short and we only need to read them, not write. But if we had a more general solution, would be nice to use it for those too.

[ericseppanen]

Perhaps we could map Postgres bool to Rust's u8 instead of bool. Not sure how to tell bindgen to do that.

I looked for a way to tell bindgen to remap bool to u8 and didn't find one. I think we'd have to write something to post-process the entire bindgen output file. Or trick it by patching the input file with #define bool uint8_t or something like that.

[ericseppanen]

Do we want to be able to convert &[u8] to &ControlFileData (and vice versa)?

Right now it looks like there's a copy happening, unless the compiler can optimize it away.

[ericseppanen]

There's an (undocumented?) standard practice in Rust, which is that whenever you use unsafe, you add a SAFETY comment explaining why this code can't cause bad things to happen. An example from the standard library:

        // SAFETY: the align is guaranteed by Rust to be a power of two and
        // the size+align combo is guaranteed to fit in our address space. As a
        // result use the unchecked constructor here to avoid inserting code
        // that panics if it isn't optimized well enough.
        unsafe { Layout::from_size_align_unchecked(size, align) }

[ericseppanen]

Should this be closed? The code was merged a while ago under another PR.

[hlinnaka]

Should this be closed? The code was merged a while ago under another PR.

Yeah, I'll close this. We had some good discussion and suggestion here, but nothing concrete pending.