fix: fix wrong 503 Service Unavailable on pg error 53400

[taimoorzaeem]

Fixes #3257.

[wolfgangwalther]

This is not a good way to do the set temp_file_limit. Since it's part of the function body, the value is not reset when the function is done. Since it's not using local it possibly leaks outside this transaction / request.

It's better to do this:

Suggested change

	create or replace function temp_file_limit()
	returns bigint as $$
	set temp_file_limit to '1MB';
	select COUNT(*) FROM generate_series('-infinity'::TIMESTAMP, 'epoch'::TIMESTAMP, INTERVAL '1 DAY');
	$$ language sql security definer;
	create or replace function temp_file_limit()
	returns bigint as $$
	select COUNT(*) FROM generate_series('-infinity'::TIMESTAMP, 'epoch'::TIMESTAMP, INTERVAL '1 DAY');
	$$ language sql security definer set temp_file_limit to '1MB';

Can you go lower than 1 MB, too? 1KB, maybe?

[taimoorzaeem]

@wolfgangwalther

grant set on parameter temp_file_limit to postgrest_test_anonymous;

create or replace function temp_file_limit()
returns bigint as $$
  select COUNT(*) FROM generate_series('-infinity'::TIMESTAMP, 'epoch'::TIMESTAMP, INTERVAL '1 DAY');
$$ language sql security definer set temp_file_limit to '1kB';

With the above, I am getting the following error on running this test:

but got:  {"code":"22023","details":null,"hint":"Valid units for this parameter are \"B\", \"kB\", \"MB\", \"GB\", 
and \"TB\".","message":"invalid value for parameter \"temp_file_limit\": \"1kb\""}

The test magically lowercases all parameter values. Same goes for 1MB to 1mb and then declaring it invalid.

[wolfgangwalther]

It works fine for me on PG15. Which version are you getting the error for?

postgres=# create or replace function temp_file_limit()
postgres-# returns bigint as $$
postgres$#   select COUNT(*) FROM generate_series('-infinity'::TIMESTAMP, 'epoch'::TIMESTAMP, INTERVAL '1 DAY');
postgres$# $$ language sql security definer set temp_file_limit to '1kB';
CREATE FUNCTION
postgres=# SELECT temp_file_limit();
ERROR:  temporary file size exceeds temp_file_limit (1kB)
CONTEXT:  SQL function "temp_file_limit" statement 1
postgres=#

Are you sure, you are not hitting #3358 (which is not merged, yet!), i.e. you are getting this because temp_file_limit is now suddenly hoisted? This would indicate another bug in the hoisting code, because apparently it doesn't keep the casing when doing so?

[taimoorzaeem]

Yes, I am sure. I am working on a different branch. Let me share the PR with you.

[wolfgangwalther]

I am working on a different branch.

This is exactly what I mean. Because you have not included the changes in #3358 here, yet, the setting will still be hoisted in this case - there is no config option to prevent it, yet.

This means that temp_file_limit will actually be set via PostgREST. And this seems to turn 1kB to 1kb. This indicates a bug in the basic hoisting feature, that #3358 changes.

[taimoorzaeem]

Can this test run with a different role, which you then make SUPERUSER for this purpose?

How do I run just a single test with a different role? Changing the role would by itself might require superuser privileges.

[taimoorzaeem]

@steve-chavez @laurenceisla Any idea on this?

[laurenceisla]

You could create another superuser role like:

CREATE ROLE postgrest_test_superuser WITH SUPERUSER;

Then grant usage on the schema, and do the request impersonating that user:

    context "test function temp_file_limit" $
      let auth = authHeaderJWT "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoicG9zdGdyZXN0X3Rlc3Rfc3VwZXJ1c2VyIiwiaWQiOiJqZG9lIn0.LQ-qx0ArBnfkwQQhIHKF5cS-lzl0gnTPI8NLoPbL5Fg" in
      it "should return http status 500" $
        request methodGet "/rpc/temp_file_limit" [auth] "" `shouldRespondWith`
          [json|{"code":"53400","message":"temporary file size exceeds temp_file_limit (1kB)","details":null,"hint":null}|]
          { matchStatus = 500 }

You can copy paste to your test, it should work as-is. Additionally, you can do a if pgVersion>=15000 for the tests (without impersonation) and the SQL you already have. Something similar to what is done in IO tests:

postgrest/test/io/fixtures.sql

Lines 22 to 27 in 9d1dc78

	DO $do$BEGIN
	IF (SELECT current_setting('server_version_num')::INT >= 150000) THEN
	ALTER ROLE postgrest_test_w_superuser_settings SET log_min_duration_sample = 12345;
	GRANT SET ON PARAMETER log_min_duration_sample to postgrest_test_authenticator;
	END IF;
	END$do$;

[taimoorzaeem]

@laurenceisla I tried this and it didn't work. I am still getting this error on all versions:

status mismatch:
         expected: 500
         but got:  403
       body mismatch:
         expected: {"code":"53400","details":null,"hint":null,"message":"temporary file size exceeds temp_file_limit (1kB)"}
         but got:  {"code":"42501","details":null,"hint":null,"message":"permission denied to set role \"postgrest_test_superuser\""}

[laurenceisla]

That's a different error, though. You need to grant that role to the authenticator here:

postgrest/test/spec/fixtures/roles.sql

Line 6 in 0d13b84

GRANT postgrest_test_anonymous, postgrest_test_default_role, postgrest_test_author TO :PGUSER;

[steve-chavez]

@taimoorzaeem This also requires an update to docs https://postgrest.org/en/v12/references/errors.html#http-status-codes