PostgREST / postgrest

REST API for any Postgres database
https://postgrest.org
MIT License
22.65k stars 1k forks source link

Fix parallel spec tests still mutating some sequences #1799

Open wolfgangwalther opened 3 years ago

wolfgangwalther commented 3 years ago

Noted after merging #1797, but unrelated to that PR, we still have some tests that depend on the order in which they are run. Some kind of rare race-condition when running the tests.

Error here: https://app.circleci.com/pipelines/github/PostgREST/postgrest/850/workflows/1c7c98b4-8081-4239-96a8-fef6a9fb5f1e/jobs/8487

simian-loco commented 2 years ago

Quick question @wolfgangwalther which may be related to this issue (but perhaps not). Is the return from prefer: return-representation / other mutating requests guaranteed to return after COMMIT has occurred (and thus after all AFTER triggers)? What about if the after trigger is DEFERRABLE?

wolfgangwalther commented 2 years ago

In fact it's completely unrelated to this issue ;)

However I made a quick test as follows:

create table t (
  c TEXT
);

create function sleep_raise() returns trigger
language plpgsql as $$begin
  perform pg_sleep(5);
  raise 'failed';
end$$;

create constraint trigger def after insert on t
deferrable initially deferred for each row
execute function sleep_raise();

Now, when I run a POST request with return=representation to this table, the request will take 5 seconds and then return with an error - and not the inserted row. So the constraint trigger is indeed executed before any response is sent.

Note, however, that this is not the case, when using Prefer: tx=rollback. The transaction will be rolled back without the constraint triggers firing. I guess we need to fix that. I'll create an issue for it.

simian-loco commented 2 years ago

Thanks for the info! I've concluded that my HTTP-based, python multi-threaded tests are most likely just running too quickly, resulting in phantom read anomalies which are throwing errors.

Before each and every test run, I create multiple user_accounts, each within a BEFORE trigger GRANTing <role> to authenticator; but sometimes an immediate, subsequent request using that new <role>'s bearer token throws a 403 complaining that authenticator does not have permission to SET ROLE <role>, I am assuming this is because of a phantom read, the GRANT not having been processed yet. I could probably fix this in a couple of ways (e.g. isolation level to serializable or smart request throttling) but for now I am just commentating the fail as most likely being anomalous and if the dev is worried to run those tests on a single thread to double check. On the single thread everything passes. IRL these requests would be rate limited anyways.

That is very cool, about the Prefer: tx=rollback. I think I have some use cases in mind for the future! I really appreciate your work on this project @wolfgangwalther, and your attentiveness to supporting it.

wolfgangwalther commented 1 month ago

Fixed a few things, right now the biggest offender is this:

https://github.com/PostgREST/postgrest/blob/01a56db7d8f1cd84fb03e9270b2cf3d187b86211/test/spec/Feature/Query/ComputedRelsSpec.hs#L107-L146