Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(wtx): Add optional support for uuid format in postgres. #290

Closed
wants to merge 0 commits into from

Conversation

humb1t
Copy link
Contributor

@humb1t humb1t commented Dec 21, 2024

In this Pull Request I want to add an optional support for Uuid type from uuid crate. I made an Encoder/Decoder implementations for it for PostgreSQL. Unfortunately there is a small problem with this code:

#[cfg(target_pointer_width = "64")]
const _: () = {
  assert!(size_of::<Error>() == 24);
};

If I add new error to the enum the wtx itself compiles fine, when feature uuid disabled, but when I used it in test project it fails. I want to ask for clarification why this code is presented and is limit of 24 bytes is a strict requirement? Thank you in advance.

@c410-f3r
Copy link
Owner

c410-f3r commented Dec 21, 2024

Привет

Thank you for the contribution. wtx is under active development and breaking changes are expected in the near future, as such, it is recommended to use more mature crates like sqlx or tokio-postgres.

This project favors performance (https://github.com/diesel-rs/metrics) and fallibility over simplicity, thus basically the reason of the assert!(size_of::<Error>() == 24); check.

The Error enum is hard-coded into a lot of functions so every time a function is called the processor "generally" allocates 24 bytes of stack memory, which might seem small but this overhead adds up from the bottom up "usually" resulting in a final significant runtime impact. Besides, embedded devices have constrained memory.

Some real world findings about the matter:

To workaround such limitation you can "box" the uuid::Error structure.

#[cfg(feature = "uuid")]
UuidError(Box<uuid::Error>),

@humb1t
Copy link
Contributor Author

humb1t commented Dec 21, 2024

Привет!

Thanks a lot @c410-f3r, I have a big enthusiasm about wtx and want to give it a try at least in some personal projects, because I was amazed by the speed it delivers. If my PRs and questions would take to much time of you and prevent regular development flow, please let me know and I will just have a synced forked version. Yet if I don't take too much time I will be honored to make small contributions from time to time.

About the restriction, it's my bad to not see that you already using a Box in some variants of Error enum. I promise to be more careful next time. It's fixed now and hope this PR can be merged.

@c410-f3r
Copy link
Owner

Erhhh... Sorry, I messed up your remote branch trying to rebase before merging. Can you push your local changes again?

Thank you for thinking that wtx is a considerable project. For what it is worth, my opinion about using other clients still stands.

If my PRs and questions would take to much time of you and prevent regular development flow, please let me know and I will just have a synced forked version. Yet if I don't take too much time I will be honored to make small contributions from time to time.

About the restriction, it's my bad to not see that you already using a Box in some variants of Error enum. I promise to be more careful next time. It's fixed now and hope this PR can be merged.

Oh, don't worry. Contributions are always welcome so feel free to provide them without hesitation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants