Introduce connection factory option for statementTimeout

Question

Introduce connection factory option for statementTimeout

alampada opened this issue 4 years ago · 15 comments

Feature Request

Is your feature request related to a problem? Please describe

I'd like to be able to set the statement timeout on connection-level.

This code below achieves the desired behavior

	return  ConnectionFactories.get(ConnectionFactoryOptions.builder()
				.option(DRIVER, "postgresql")
				.option(HOST, "172.17.0.2")
				.option(USER, "x")
				.option(PASSWORD, "x")
				.option(Option.valueOf("options"), Map.of("statement_timeout", "1000"))
				.build());

Describe the solution you'd like

As per the discussion in r2dbc/r2dbc-spi#173 (comment), it would be nice to be able to set the statement_timeout option through the connection url.

Describe alternatives you've considered

Workaround above.
Handling timeouts with Flux/Mono timeouts instead of statement_timeout, but as far as I understand the Mono/Flux timeout approach doesn't cancel the query.

Teachability, Documentation, Adoption, Migration Strategy

Users will be able to set the statement_timeout in the connection URL, for example in the spring configuration files instead of having to write code. Readme needs to update to reflect the change.

Answer 1 · 2021-02-03T14:27:10.000Z

I am willing to work on this.

As part of this feature, are we planning to introduce 2 things: PostgresqlConnectionFactory#statementTimeout and PostgresqlConnectionFactory.option(STATEMENT_TIMEOUT, ...)?

or only via option?

Answer 2 · 2021-02-03T14:47:19.000Z

On PostgresqlConnectionFactory, we don't configure the timeout. Do you rather mean PostgresqlConnectionConfiguration? Other than that, any help is welcome. Let us know if you have any questions.

Answer 3 · 2021-02-03T16:29:18.000Z

Ah! yes PostgresqlConnectionConfiguration

If I'm not mistaken, we need to add a builder method PostgresqlConnectionConfiguratio#statementTimeout(...) right?

Answer 4 · 2021-02-04T08:39:39.000Z

Actually, it's the builder for the config class.

Answer 5 · 2021-02-08T15:30:36.000Z

Thanks, Mark. I have a couple of questions.

Should the method accept Duration or String type ?
After creating the builder method, do I need to configure/set statement_timeout somewhere? I am unable to figure out where it's configured.

Answer 6 · 2021-02-08T16:35:13.000Z

We should define the property as Option<Duration>, similar to connection timeout. On the side that accepts the value, we could allow for Duration and String values (PT1S), we have already converters for other data types in place.

I'm not sure whether we need to issue a SET statement_timeout = … statement or whether we can pass on the option in the startup packet along with other options. Paging @davecramer.

Answer 7 · 2021-02-18T17:08:40.000Z

Hi @davecramer, could you please advise me on the above queries.

Answer 8 · 2021-06-09T06:36:16.000Z

hi, @mp911de could you please review the PR?!

looks like we need to send these parameters in StartUpMessage itself Jdbc-Connect.md

Answer 9 · 2021-06-09T07:17:45.000Z

Thanks for submitting a PR. I've been on vacation recently and didn't get to review recent pull requests. AFAIK, Postgres allows setting quite a few variables in the startup so that it doesn't require any subsequent SET … interactions.

While already at it, care to add also methods to PostgresqlConnection to set lock/statement timeouts through Duration? These would translate to set statement_timeout=… calls. There's already a specification request in the SPI (r2dbc/r2dbc-spi#173) and we would basically implement what's planned anyway.

Answer 10 · 2021-06-09T08:04:32.000Z

Hi,

Thank you for looking into this.

I'd like to share that after further investigation, we were able to set the statement_timeout in the url using this format: r2dbc:postgresql://172.17.0.2:5432/postgres?options=statement_timeout=1s. Based on this, what are your thoughts in terms of documenting the current behaviour instead of doing code changes?

Answer 11 · 2021-06-09T08:55:27.000Z

In terms of the connection factory option statement_timeout, it always makes sense to align with PGJDBC so URLs remain interchangeable where possible.

We document connection factory options in the readme, see https://github.com/pgjdbc/r2dbc-postgresql#getting-started

Answer 12 · 2021-06-14T17:21:33.000Z

Thanks for submitting a PR. I've been on vacation recently and didn't get to review recent pull requests. AFAIK, Postgres allows setting quite a few variables in the startup so that it doesn't require any subsequent SET … interactions.

While already at it, care to add also methods to PostgresqlConnection to set lock/statement timeouts through Duration? These would translate to set statement_timeout=… calls. There's already a specification request in the SPI (r2dbc/r2dbc-spi#173) and we would basically implement what's planned anyway.

Sure. Let me add these 2 methods to PostgresqlConnection. Please correct me If I am wrong, should these two APIs return Mono<Void>?

Mono<Void> lockTimeout(Duration lockTimeout)
Mono<Void> statementTimeout(Duration statementTimeout)

Answer 13 · 2021-06-14T19:44:24.000Z

Exactly. We just want to await completion.

Answer 14 · 2021-06-20T12:07:13.000Z

Added these 2 APIs!

Answer 15 · 2021-07-07T12:38:43.000Z