feat(postgres): add an option to specify extra options

[liushuyu]

This Pull Request allows you to specify extra command options to the PostgreSQL server through connection options like search path and statement timeouts etc.

This could fix #1290.

[abonander]

The options parameter of StartupMessage is deprecated:

(This is deprecated in favor of setting individual run-time parameters.)

I would prefer if the interface was something like:

pub fn options<K, V, I>(self, options: I) -> Self 
where
    K: ToString,
    V: ToString,
    I: IntoIterator<Item = (K, V)>,
{
    // ...
}

As for the URL format, how about something like ?options[key1]=value1&options[key2]=value2 instead of URL-encoding the options string wholesale?

[liushuyu]

The options parameter of StartupMessage is deprecated:

(This is deprecated in favor of setting individual run-time parameters.)

Sorry, is this in SQLx or in the PostgreSQL? So if we don't use options, what should we use to set options like schema_path or stmt_timeout?

I would prefer if the interface was something like:

pub fn options<K, V, I>(self, options: I) -> Self 
where
    K: ToString,
    V: ToString,
    I: IntoIterator<Item = (K, V)>,
{
    // ...
}

I see. I will convert the code to do that.

As for the URL format, how about something like ?options[key1]=value1&options[key2]=value2 instead of URL-encoding the options string wholesale?

This is per https://www.postgresql.org/docs/13/libpq-connect.html#LIBPQ-CONNSTRING. PostgreSQL specified that the client should encode the URL like this. The same applies to the PGOPTIONS environment variable, which is so specified in the PostgreSQL documentation.

[abonander]

Sorry, is this in SQLx or in the PostgreSQL? So if we don't use options, what should we use to set options like schema_path or stmt_timeout?

This is in Postgres (scroll down to StartupMessage (F)): https://www.postgresql.org/docs/current/protocol-message-formats.html

The options can be passed directly as extra key-value pairs in the StartupMessage, so in PgConnectOptions I would add a HashMap or something to collect the extra parameters.

This is per https://www.postgresql.org/docs/13/libpq-connect.html#LIBPQ-CONNSTRING. PostgreSQL specified that the client should encode the URL like this. The same applies to the PGOPTIONS environment variable, which is so specified in the PostgreSQL documentation.

We're really not beholden to that besides making an attempt to support the same connection string options as libpq. In https://www.postgresql.org/docs/current/config-setting.html#id-1.6.7.4.5 it also states:

Other clients and libraries might provide their own mechanisms, via the shell or otherwise, that allow the user to alter session settings without direct use of SQL commands.

What my ideal would be is to support both formats, both the libpq style for compatibility as well as my suggested format for better ergonomics, so both these would be allowed, and functionally equivalent:

postgresql://user@localhost/?options=-c%20geqo%3Doff%20-c%20statement_timeout%3D5min
postgresql://user@localhost/?options[geqo]=off&options[statement_timeout]=5min

In that case, it doesn't really make sense to try to parse the options string format when we can pass it directly to Postgres, so I'd just use the options parameter of StartupMessage regardless of it being deprecated, and also convert the friendlier format to the options string format internally.

[liushuyu]

Sorry, is this in SQLx or in the PostgreSQL? So if we don't use options, what should we use to set options like schema_path or stmt_timeout?

This is in Postgres (scroll down to StartupMessage (F)): https://www.postgresql.org/docs/current/protocol-message-formats.html

The options can be passed directly as extra key-value pairs in the StartupMessage, so in PgConnectOptions I would add a HashMap or something to collect the extra parameters.

This is per https://www.postgresql.org/docs/13/libpq-connect.html#LIBPQ-CONNSTRING. PostgreSQL specified that the client should encode the URL like this. The same applies to the PGOPTIONS environment variable, which is so specified in the PostgreSQL documentation.

We're really not beholden to that besides making an attempt to support the same connection string options as libpq. In https://www.postgresql.org/docs/current/config-setting.html#id-1.6.7.4.5 it also states:

Other clients and libraries might provide their own mechanisms, via the shell or otherwise, that allow the user to alter session settings without direct use of SQL commands.

What my ideal would be is to support both formats, both the libpq style for compatibility as well as my suggested format for better ergonomics, so both these would be allowed, and functionally equivalent:

postgresql://user@localhost/?options=-c%20geqo%3Doff%20-c%20statement_timeout%3D5min
postgresql://user@localhost/?options[geqo]=off&options[statement_timeout]=5min

In that case, it doesn't really make sense to try to parse the options string format when we can pass it directly to Postgres, so I'd just use the options parameter of StartupMessage regardless of it being deprecated, and also convert the friendlier format to the options string format internally.

Sorry, I didn't research enough, and you are right.

I will attempt to adapt the code to your suggestions.

[liushuyu]

I have adapted the code to @abonander 's suggestions.

However, I am having difficulties coming up with a more robust libpq style option string parser. The current implementation is a only "just works" one. Suggestions welcome.

In addition, I feel like the options could be typed as Vec<(String, String)>? The suggested HashMap<String, String> is bit inconvient when passing the parsed results from a connection string.

[abonander]

@liushuyu sorry my last message was probably pretty confusing, I was thinking out loud.

If we want to support options as a single string in the URL then it doesn't make any sense to me to parse it when we could pass it directly to Postgres as the options field in StartupMessage. After thinking about it some more, it doesn't really matter that the field is deprecated because they don't seem to have any plans to actually remove it, and I'm not sure how they could remove it without potentially breaking a bunch of existing clients.

So go ahead and go back to what you were doing, although I would keep the change to the options() method signature and have it generate the options string internally.

[liushuyu]

@liushuyu sorry my last message was probably pretty confusing, I was thinking out loud.

If we want to support options as a single string in the URL then it doesn't make any sense to me to parse it when we could pass it directly to Postgres as the options field in StartupMessage. After thinking about it some more, it doesn't really matter that the field is deprecated because they don't seem to have any plans to actually remove it, and I'm not sure how they could remove it without potentially breaking a bunch of existing clients.

So go ahead and go back to what you were doing, although I would keep the change to the options() method signature and have it generate the options string internally.

Ah, I see. I will make these changes.

[liushuyu]

I have adapted the code to the latest suggestions. There are rough edges in the command formatting where I think I need to observe the handling of certain edge-cases by psql client (e.g. excessive whitespace, whitespace in the arguments)

[liushuyu]

Thank you for reviewing my changes. I have rebased my branch and fixed the formatting.

[liushuyu]

Is there anything left to do to get this pull request approved and merged?

[abonander]

Looks good, just a few nits left.

[liushuyu]

Thank you for the suggestions. Comments addressed and commits rebased.

[kevincox]

This appears to have a bug where if multiple options are provided the -c is tacked onto the previous one. This can be worked around by adding the space manually.

		.options([
			("default_transaction_isolation", "serializable "),
			("idle_in_transaction_session_timeout", "5min "),
			("statement_timeout", "2s"),
		]);

Or by calling this multiple times.

[kevincox]

Bug filed: #1730