Escape PostgreSQL options

[V02460]

Properly escape PostgreSQL options containing spaces and backslash characters as specified under https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-OPTIONS.

Is this a breaking change?

No, this change fixes behavior that I consider a minor edge case.

Yes,

anyone who's already manually escaping options will get different, likely incorrect, results after this patch

[abonander]

This bothers me for a few reasons:

• Formats the string and escapes it afterward
• Allocates a whole string just to throw it away
• Pushes to options_str character-by-character
  • This especially pessimizes the case where no escaping is needed.

The latter two both serve to create unnecessary allocation pressure. While this isn't a super hot routine, a lot of little allocations tend to increase heap fragmentation, which is compounded by the number of options being passed to a connection, the lengths of the values, and the number of connections that are opened over the lifetime of the application.

With just an extra ten lines of code, it's possible to avoid the intermediate allocation and write the whole string at once if there are no characters to escape: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0b0bb2909484f9578c2e4c02ed1f41b5

[abonander]

You would use it like this:

use std::fmt::Write;

// Writing to a string is infallible.
write!(options_str, "-c {}={}", PgEscapeOption(k), PgEscapeOption(v)).ok();

[V02460]

Thank you for the quick and thorough feedback! I like your suggestion and tried to apply it – but:

The key and value parameters of the options function are of the Display trait. To avoid heap allocations your solution would have to be adopted to struct PgEscapeOption<T: Display>(T);. I don’t think that is possible.

One can sidestep the problem by changing the parameters of the options function to take a string instead, so their backing buffer can be accessed. However, that would slightly restrict the user in what they can pass in.

pub fn options<'a, I>(mut self, options: I) -> Self
    where
        I: IntoIterator<Item = (&'a str, &'a str)>,
    {

Formats the string and escapes it afterward

I don’t think that does any harm on its own. I agree with the other points.

[abonander]

The key and value parameters of the options function are of the Display trait. To avoid heap allocations your solution would have to be adopted to struct PgEscapeOption<T: Display>(T);. I don’t think that is possible.

I realized that only late last night. It is possible, you just need to drop down a layer: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=a8a59849ca9c7d1f3a76d4bfe1ab05f8

Not quite as elegant but still avoids the intermediate allocations.

I don’t think that does any harm on its own. I agree with the other points.

No, it's just a little weird.

[abonander]

This is technically a breaking behavior change as well, because anyone who's already manually escaping options will get different, likely incorrect, results after this patch.

This should also document that the strings will be escaped:

sqlx/sqlx-postgres/src/options/mod.rs

Lines 496 to 504 in e474be6

	/// Set additional startup options for the connection as a list of key-value pairs.
	///
	/// # Example
	///
	/// ```rust
	/// # use sqlx_postgres::PgConnectOptions;
	/// let options = PgConnectOptions::new()
	/// .options([("geqo", "off"), ("statement_timeout", "5min")]);
	/// ```

Alternatively, we don't do this internally but instead expose PgEscapeOption and direct people to use it. That would be backwards-compatible.

[V02460]

Alternatively, we don't do this internally but instead expose PgEscapeOption and direct people to use it. That would be backwards-compatible.

This would leak implementation details and keep a foot gun around, so I’d caution against that.

[abonander]

This would leak implementation details and keep a foot gun around, so I’d caution against that.

1. It's a detail of the underlying protocol, which can be important to understand.
2. We could emit a warning log if someone provides a string that isn't properly escaped.
3. I have learned that silent changes to behavior tend to trip people up regardless, because not everyone reads the CHANGELOG or reads it thoroughly. Hyrum's Law has always held true.
4. Users who aren't prepared to upgrade to a new major version won't be able to benefit.

If you're fine waiting for this to hit 0.9.0, then it doesn't matter.