postgrest icon indicating copy to clipboard operation
postgrest copied to clipboard

feat: add config db-hoisted-tx-settings to allow only hoisted function settings

Open taimoorzaeem opened this issue 1 year ago • 4 comments

Fixes #3242.

@steve-chavez @laurenceisla @wolfgangwalther Even though I have applied the filter for function settings using the new config, but the setting is still being applied in io tests. Am I testing it right?

taimoorzaeem avatar Mar 29 '24 07:03 taimoorzaeem

It appears that the test case is failing, however, the test passes when run with postgrest-test-io -k test_only_hoisted_settings_are_applied.

taimoorzaeem avatar Apr 01 '24 08:04 taimoorzaeem

It appears that the test case is failing, however, the test passes when run with postgrest-test-io -k test_only_hoisted_settings_are_applied.

You could try to make the RPC plpgsql instead of SQL to make sure to avoid any inlining - although that should not happen in this case anyway.

Or maybe some other test sets the statement timeout to 5s? What if you change the timeout on the RPC to e.g. 6s? Does wrong expectation change as well? Then you'd be sure that this is actually the value we are talking about.

Not exactly sure what happens.

wolfgangwalther avatar Apr 02 '24 20:04 wolfgangwalther

Or maybe some other test sets the statement timeout to 5s? What if you change the timeout on the RPC to e.g. 6s? Does wrong expectation change as well? Then you'd be sure that this is actually the value we are talking about.

Thanks for this suggestion. I tried the changing the value to 6s and 7s and it still showed 5s at test time.

My guess is that statement_timeout has default values of 5s when all io-tests are run and 2s when an individual io-test is run with -k flag. I've changed the value to 5s to indicate default value in the test.

taimoorzaeem avatar Apr 05 '24 09:04 taimoorzaeem

My guess is that statement_timeout has default values of 5s when all io-tests are run and 2s when an individual io-test is run with -k flag. I've changed the value to 5s to indicate default value in the test.

The more likely explanation is that some other test is leaking it's settings.

Right, we have this in another test:

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

This test needs to clean up behind and reset the timeout...

wolfgangwalther avatar Apr 05 '24 09:04 wolfgangwalther

To unblock v12.2.0 (https://github.com/PostgREST/postgrest/issues/3501), just rebased and addressed the latest reviews.

steve-chavez avatar May 21 '24 20:05 steve-chavez