Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
May 22, 2024

Conversation

taimoorzaeem
Copy link
Collaborator

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?

CHANGELOG.md Outdated Show resolved Hide resolved
test/io/configs/defaults.config Outdated Show resolved Hide resolved
test/io/fixtures.sql Outdated Show resolved Hide resolved
@taimoorzaeem taimoorzaeem force-pushed the set_allowlist branch 2 times, most recently from f817e2e to baaa975 Compare March 31, 2024 21:00
@taimoorzaeem taimoorzaeem changed the title fix: breaking change for RPCs with SET (#3242) feat: add config db-hoisted-tx-settings to allow only hoisted function settings (#3242) Apr 1, 2024
@taimoorzaeem taimoorzaeem changed the title feat: add config db-hoisted-tx-settings to allow only hoisted function settings (#3242) feat: add config db-hoisted-tx-settings to allow only hoisted function settings Apr 1, 2024
@taimoorzaeem
Copy link
Collaborator Author

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.

@wolfgangwalther
Copy link
Member

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.

Copy link
Member

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like you need to update the pytest snapshot for schema cache, too.

test/io/test_io.py Outdated Show resolved Hide resolved
test/io/test_io.py Outdated Show resolved Hide resolved
@taimoorzaeem
Copy link
Collaborator Author

taimoorzaeem commented Apr 5, 2024

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.

@wolfgangwalther
Copy link
Member

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...

@steve-chavez
Copy link
Member

To unblock v12.2.0 (#3501), just rebased and addressed the latest reviews.

@@ -23,6 +23,7 @@ ALTER ROLE db_config_authenticator SET pgrst.openapi_server_proxy_uri = 'https:/
ALTER ROLE db_config_authenticator SET pgrst.server_cors_allowed_origins = 'http://origin.com';
ALTER ROLE db_config_authenticator SET pgrst.server_timing_enabled = 'false';
ALTER ROLE db_config_authenticator SET pgrst.server_trace_header = 'CF-Ray';
ALTER ROLE db_config_authenticator SET pgrst.db_hoisted_tx_settings = 'work_mem';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the last one before merging. The comment mentions:

-- these settings will override the values in configs/no-defaults.config, so they must be different

So this should be different from the value in no-defaults.config to test that it's actually overriding it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok done.

Btw this whole workflow is double plus ungood, adding a config option is always painful.

This and #3481 (comment) tells us the whole Config handling needs reworking.

@steve-chavez steve-chavez merged commit 8cbcf98 into PostgREST:main May 22, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Regression / breaking change for RPCs with SET
4 participants