test: new tables that don't use the scache are not immediately available - #5121
test: new tables that don't use the scache are not immediately available#5121steve-chavez wants to merge 2 commits into
Conversation
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
- 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.
There was a problem hiding this comment.
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.
|
CI is failing with some weird error: Looks like hackage is down |
|
Hackage is up again so all the tests pass now |
3 participants
Proves the problem in #4613.