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

Allowed CStr to be used as SPI commands #1864

Merged
merged 3 commits into from
Sep 21, 2024

Conversation

YohDeadfall
Copy link
Contributor

@YohDeadfall YohDeadfall commented Sep 17, 2024

Made with love to those who doesn't like allocations ❤️

Since the Rust version 1.77 there's a possibility to use c and cr prefixes to make C string literals. That requires a little bit more characters to type, but reduces the number of allocations and therefore boosts pgrx extensions opted in. The feature isn't breaking due to being generic based.


fn execute(
/// A trait representing a query which can be prepared.
pub trait PreparableQuery<'conn>: Query<'conn> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably seems obvious but can you explain how this distinguishes from Query?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To distinguish from the prepared statements which should not be used by the prepare methods of the SPI client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, not entirely sure what the benefit is?

Copy link
Member

@workingjubilee workingjubilee Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably obvious to you, it's just been ages since I've looked at the Query API and my main dread is it being cemented further, because I would rather it... not exist, tbh. Not in its current form anyway. It's... overly cutesy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem that trait solves is prohibiting PreparedStatement<'conn> from being used with prepare and prepare_mut methods of the SqlClient<'conn>, so the following code will result in a compiler error:

let conn = SpiConnection::connect()?;
let client = conn.client();

let once = client.prepare(c"SELECT 1", None)?;
let twice = client.prepare(once, None);
//                 ------- ^^^^ the trait `PreparableQuery<'_>` is not implemented for `PreparedStatement<'_>`
//                              |
//                              required by a bound introduced by this call

Copy link
Contributor Author

@YohDeadfall YohDeadfall Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played with the CI and observed that my UI test fails only for PostgreSQL versions 12, 13 and 14, but not for others. That's totally weird. See #1870.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...huh.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...oh, I remember. They introduced a type named String in PostgreSQL. Can you defer the test to opening an issue instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@workingjubilee
Copy link
Member

Thank you~!

@workingjubilee workingjubilee merged commit 4cd1358 into pgcentralfoundation:develop Sep 21, 2024
28 checks passed
@YohDeadfall YohDeadfall deleted the cstr-as-query branch September 21, 2024 09:06
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