feat: add db-pool-automatic-recovery configuration to disable connection retrying

[taimoorzaeem]

This should close #1614.

@steve-chavez I haven't written any tests because I am not sure exactly how configurations are being tested here. And so Codecov is complaining. However, I have tested it locally by giving it an invalid db-uri and when the pg server is not available for which recovery disables successfully. Please review.

[taimoorzaeem]

@steve-chavez Also, I am thinking maybe we should call the configuration option disable-retrying instead of disable-recovery. It seems more consistent with the internal identifiers and as well as the error messages.

[steve-chavez]

@taimoorzaeem Since recovery is a feature of the connection pool, how about calling it db-pool-automatic-recovery? It would be true by default.

Note that all our pool configs have the db-pool prefix.

[steve-chavez]

However, I have tested it locally by giving it an invalid db-uri and when the pg server is not available for which recovery disables successfully.

@taimoorzaeem Hm, I would expect the disable to work in this case:

$ PGRST_DB_POOL_AUTOMATIC_RECOVERY="false" postgrest-with-postgresql-15 postgrest-run

postgrest-with-postgresql-15: You can connect with: psql 'postgres:///postgres?host=/run/user/1000/postgrest/postgrest-with-postgresql-15-OV7/socket' -U postgres
postgrest-with-postgresql-15: You can tail the logs with: tail -f /run/user/1000/postgrest/postgrest-with-postgresql-15-OV7/db.log
14/Aug/2023:14:33:05 -0500: Starting PostgREST 11.2.0 (bd5e361)...

$ psql 'postgres:///postgres?host=/run/user/1000/postgrest/postgrest-with-postgresql-15-OV7/socket' -U postgres

select pg_terminate_backend(pid) from pg_stat_activity  where application_name iLIKE '%postgrest%';
 pg_terminate_backend 
----------------------
 t

But it still recovers:

FATAL:  terminating connection due to administrator command
14/Aug/2023:14:36:04 -0500: Retrying listening for notifications on the pgrst channel..
14/Aug/2023:14:36:04 -0500: Starting PostgREST 11.2.0 (bd5e361)...

For this I think we can catch the exception here and also kill the main thread:

postgrest/src/PostgREST/AppState.hs

Lines 144 to 150 in 87d6a0d

	usePool :: AppState -> SQL.Session a -> IO (Either SQL.UsageError a)
	usePool AppState{..} x = do
	res <- SQL.use statePool x
	whenLeft res (\case
	SQL.AcquisitionTimeoutUsageError -> debounceLogAcquisitionTimeout -- this can happen rapidly for many requests, so we debounce
	_ -> pure ())
	return res

---

Is the env var not getting applied? I've tried:

$ PGRST_DB_POOL_AUTOMATIC_RECOVERY="false" postgrest-run -- --dump-config | grep automatic -c
0

---

The new config is missing from:

postgrest/src/PostgREST/CLI.hs

Lines 124 to 166 in 87d6a0d

	exampleConfigFile :: [Char]
	exampleConfigFile =
	[str|## Admin server used for checks. It's disabled by default unless a port is specified.
	|# admin-server-port = 3001
	|
	|## The database role to use when no client authentication is provided
	|# db-anon-role = "anon"
	|
	|## Notification channel for reloading the schema cache
	|db-channel = "pgrst"
	|
	|## Enable or disable the notification channel
	|db-channel-enabled = true
	|
	|## Enable in-database configuration
	|db-config = true
	|
	|## Function for in-database configuration
	|## db-pre-config = "postgrest.pre_config"
	|
	|## Extra schemas to add to the search_path of every request
	|db-extra-search-path = "public"
	|
	|## Limit rows in response
	|# db-max-rows = 1000
	|
	|## Allow getting the EXPLAIN plan through the `Accept: application/vnd.pgrst.plan` header
	|# db-plan-enabled = false
	|
	|## Number of open connections in the pool
	|db-pool = 10
	|
	|## Time in seconds to wait to acquire a slot from the connection pool
	|# db-pool-acquisition-timeout = 10
	|
	|## Time in seconds after which to recycle pool connections
	|# db-pool-max-lifetime = 1800
	|
	|## Time in seconds after which to recycle unused pool connections
	|# db-pool-max-idletime = 30
	|
	|## Stored proc to exec immediately after auth
	|# db-pre-request = "stored_proc_name"

---

I haven't written any tests because I am not sure exactly how configurations are being tested here. And so Codecov is complaining.

Here's an example of how configs are modified per io test:

postgrest/test/io/test_io.py

Lines 1056 to 1061 in 87d6a0d

	def test_succeed_w_role_having_superuser_settings(defaultenv):
	"Should succeed when having superuser settings on the impersonated role"
	env = {**defaultenv, "PGRST_DB_CONFIG": "true", "PGRST_JWT_SECRET": SECRET}
	with run(stdin=SECRET.encode(), env=env) as postgrest:

Here's an example that catches the exit code:

postgrest/test/io/test_io.py

Lines 69 to 75 in 87d6a0d

	def test_fail_with_invalid_password(defaultenv):
	"Connecting with an invalid password should fail without retries."
	uri = f'postgresql://?dbname={defaultenv["PGDATABASE"]}&host={defaultenv["PGHOST"]}&user=some_protected_user&password=invalid_pass'
	env = {**defaultenv, "PGRST_DB_URI": uri}
	with run(env=env, wait_for_readiness=False) as postgrest:
	exitCode = wait_until_exit(postgrest)
	assert exitCode == 1

Might be possible to have an io test based on those

[taimoorzaeem]

For this I think we can catch the exception here and also kill the main thread:

postgrest/src/PostgREST/AppState.hs

Lines 144 to 150 in 87d6a0d

	usePool :: AppState -> SQL.Session a -> IO (Either SQL.UsageError a)
	usePool AppState{..} x = do
	res <- SQL.use statePool x
	whenLeft res (\case
	SQL.AcquisitionTimeoutUsageError -> debounceLogAcquisitionTimeout -- this can happen rapidly for many requests, so we debounce
	_ -> pure ())
	return res

@steve-chavez This didn't work. Alternatively, I handled that in listener function.

[laurenceisla]

The static build is failing, maybe related to #2905? Rebasing with the main branch should fix this.

[taimoorzaeem]

@steve-chavez @laurenceisla I was thinking of adding a test case for select pg_terminate_backend(pid) from pg_stat_activity where application_name iLIKE '%postgrest%';. Can we make rpc calls from IO tests? I am thinking of adding the above sql query to a stored procedure and then make a rpc call to test the functionality. The code coverage may also be complete after this.

[steve-chavez]

@taimoorzaeem Yes, that's possible. There are some examples on test_io.py:

postgrest/test/io/fixtures.sql

Lines 118 to 123 in 32b77ca

	create function change_role_statement_timeout(timeout text) returns void as $_$
	begin
	execute format($$
	alter role current_user set statement_timeout = %L;
	$$, timeout);
	end $_$ volatile language plpgsql ;

postgrest/test/io/test_io.py

Lines 853 to 857 in 32b77ca

	# reload statement_timeout with NOTIFY
	response = postgrest.session.post(
	"/rpc/change_role_statement_timeout", data={"timeout": "5s"}
	)
	assert response.status_code == 204

[taimoorzaeem]

@steve-chavez I have tried doing the above in the latest push, however calling such an rpc immediately kills postgrest and an exception is thrown, which the code coverage does not detect.

[steve-chavez]

@taimoorzaeem Nice work! How about documenting this new config on:

• https://postgrest.org/en/stable/references/connection_pool.html#automatic-recovery
• https://postgrest.org/en/stable/references/configuration.html#list-of-parameters

Repo at https://github.com/PostgREST/postgrest-docs.

[taimoorzaeem]

@taimoorzaeem Nice work! How about documenting this new config on:

Sure! I would love to. :)

[yaobinwen-mvs]

Thank you all for the effort to make it happen! Really appreciate it! Kindly ask do you know in which version will this feature be included and released?

[taimoorzaeem]

@yaobinwen-mvs Cheers! Most likely, we will have this feature in our next release which would be PostgREST 11.3.0 if we do a minor release.

[yaobinwen-mvs]

@taimoorzaeem Cool! Thanks! Do you know when the new version would be released? (I looked around in the repository but didn't seem to find any document about the release schedule.)

[steve-chavez]

@yaobinwen-mvs We'll make a new release once we fix #2846, which should be completed in the next few weeks.