Skip to content

test: new tables that don't use the scache are not immediately available #5121

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.

Sign up for GitHub

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

Closed
steve-chavez wants to merge 2 commits into from
Closed

test: new tables that don't use the scache are not immediately available #5121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32198fd
test: reuse psql_as_superuser in tests
steve-chavez Jul 23, 2026
77efb2f
test: new tables that don't use the scache are not immediately available
steve-chavez Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions test/io/fixtures/big_schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11410,3 +11410,41 @@ TO postgrest_test_anonymous;
create or replace function apflora.notify_pgrst() returns void as $$
notify pgrst;
$$ language sql;

CREATE SCHEMA bigdata;

GRANT ALL ON SCHEMA bigdata TO postgrest_test_anonymous;
ALTER DEFAULT PRIVILEGES FOR ROLE postgres IN SCHEMA bigdata
GRANT SELECT ON TABLES TO PUBLIC;

CREATE OR REPLACE PROCEDURE bigdata.create_tables(
table_count integer
) LANGUAGE plpgsql AS $$
DECLARE
table_idx integer;
batch_size integer := 500;
BEGIN
FOR table_idx IN 1..table_count LOOP
EXECUTE format('CREATE TABLE bigdata.data_%s (col text)', table_idx);

-- This batch_size is to avoid the error: HINT: You might need to increase "max_locks_per_transaction".
IF table_idx % batch_size = 0 THEN
COMMIT;
END IF;
END LOOP;
NOTIFY pgrst;
END;
$$;

CREATE OR REPLACE FUNCTION bigdata.create_random_table() RETURNS text AS $$
DECLARE
table_name text := format(
'new_data_%s',
substring(md5(clock_timestamp()::text || random()::text) from 1 for 12)
);
BEGIN
EXECUTE format('CREATE TABLE bigdata.%I (col text)', table_name);
NOTIFY pgrst;
RETURN table_name;
END;
$$ LANGUAGE plpgsql;
27 changes: 27 additions & 0 deletions test/io/test_big_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import pytest
import requests

from util import psql_as_superuser
from postgrest import run


Expand Down Expand Up @@ -120,3 +121,29 @@ def test_second_request_for_non_existent_table_should_be_quick(defaultenv):
first_duration = response.elapsed.total_seconds()
response = postgrest.session.get("/unknown-table")
assert response.elapsed.total_seconds() < first_duration / 2


def test_new_table_is_immediately_available(defaultenv):
"new table that don't use the schema cache should be immediately available"

psql_as_superuser("CALL bigdata.create_tables(5000);")

env = {
**defaultenv,
"PGRST_DB_SCHEMAS": "bigdata",
"PGRST_DB_POOL": "2",
"PGRST_DB_ANON_ROLE": "postgrest_test_anonymous",
}

with run(env=env, wait_max_seconds=30) as postgrest:
response = postgrest.session.get("/data_1?col=eq.1")
assert response.status_code == 200

response = postgrest.session.post("/rpc/create_random_table")
assert response.status_code == 200
table_name = response.json()

response = postgrest.session.get(f"/{table_name}?col=eq.1")
if response.status_code == 404:
pytest.xfail("new table returns 404 ")
assert response.status_code == 200
Comment on lines +126 to +149

@steve-chavez steve-chavez Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The main idea here is:

  • Create a good amount of tables at the beginning, so the scache load is slow
  • Create a random table (including notify pgrst) and obtain the name.
  • Request the new table and watch it take some time to stop replying with 404.

I manually tested this with 120K tables (mentioned on #4462, same case as #4613) and was able to see the request failing with 404 for a good time. But for test speed 5000 was enough.

Also I load this bunch of tables on a procedure at the beginning to avoid slowing down the other tests.

@wolfgangwalther wolfgangwalther Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The main idea here is:

  • Create a good amount of tables at the beginning, so the scache load is slow
  • Create a random table (including notify pgrst) and obtain the name.
  • Request the new table and watch it take some time to stop replying with 404.

I did not look at the code at all, but this reads an awful lot like a test that will be heavily timing dependent... which I am really not looking forward to.

@steve-chavez steve-chavez Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I guess I wasn't clear but no, there's no timing (no sleep call) in the test right now and won't expect it to be after the fix.

If #5120 is merged this is expected to pass as it is (without xfail).

@wolfgangwalther wolfgangwalther Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there's no timing (no sleep call) in the test

"timing dependent" does not mean "it has a sleep call". It means that whether the test passes or fails depends on how fast the machine running the test operates.

Run this test on a machine that does not make "the scache load [...] slow" and you might not have the desired test result. That's the problematic timing dependence.

@steve-chavez steve-chavez Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The test does accurately capture what #4613 is about.

To not make it timing dependent I guess we need to capture if a simple request like /table?id=eq.1 doesn't use the schema cache. For that we'd need some schema cache metric like schema_cache_use but it doesn't seem generally useful or worth it.

Any other ideas?

@wolfgangwalther wolfgangwalther Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't we just need to:

  • load the schema cache
  • create a new table dynamically, without reloading the schema cache
  • make a request

That should already show the "problem", right?

@taimoorzaeem taimoorzaeem Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • load the schema cache
  • create a new table dynamically, without reloading the schema cache
  • make a request

Yeah, this should be it. I am also confused on why we are creating a lot of tables.

@steve-chavez steve-chavez Jul 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fair enough, I can add a simpler test.

Although this simple test doesn't prove a subproblem of #4613, that is that with a big number of tables the scache load takes a good amount of time leading to that 404. If the number of tables was small this problem wouldn't be noticeable.

I added some setup code here to prove that and I think that's valuable but I can't see an immediate need for it.

So I'll just open a new PR and leave this one as is in case this setup can be reused later.

43 changes: 10 additions & 33 deletions test/io/test_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import os
import re
import signal
import subprocess
import time
import pytest
import requests
Expand All @@ -16,6 +15,7 @@
relativeSeconds,
drain_stdout,
match_log,
psql_as_superuser,
)
from postgrest import (
Admin,
Expand All @@ -33,20 +33,6 @@
)


def psql_as_superuser(query):
subprocess.check_call(
[
"psql",
"--username",
"postgres",
"--set",
"ON_ERROR_STOP=1",
"-c",
query,
]
)


def test_connect_with_dburi(dburi, defaultenv):
"Connecting with db-uri instead of LIPQ* environment variables should work."
defaultenv_without_libpq = {
Expand Down Expand Up @@ -684,24 +670,15 @@ def test_listener_query_is_visible_in_pg_stat_activity(defaultenv):
}

with run(env=env):
query = """
select query
from pg_stat_activity
where application_name = 'listener-query-test'
and query = 'LISTEN "pgrst"'
limit 1;
"""
output = subprocess.check_output(
[
"psql",
"--set",
"ON_ERROR_STOP=1",
"--tuples-only",
"--no-align",
"-c",
query,
],
text=True,
output = psql_as_superuser(
"""
select query
from pg_stat_activity
where application_name = 'listener-query-test'
and query = 'LISTEN "pgrst"'
limit 1;
""",
capture_output=True,
).strip()

assert output == 'LISTEN "pgrst"'
Expand Down
19 changes: 19 additions & 0 deletions test/io/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import re
import threading
import jwt
import subprocess
from datetime import datetime, timedelta, timezone


Expand Down Expand Up @@ -75,3 +76,21 @@ def parse_server_timings_header(header):
_, duration = duration_text.split("=")
timings[name.strip()] = float(duration)
return timings


def psql_as_superuser(query, capture_output=False):
cmd = [
"psql",
"--username",
"postgres",
"--set",
"ON_ERROR_STOP=1",
]
if capture_output:
cmd.extend(["--tuples-only", "--no-align"])
cmd.extend(["-c", query])

if capture_output:
return subprocess.check_output(cmd, text=True)

subprocess.check_call(cmd)