ci: fix flaky prepared statements test#1026
Conversation
This comment has been minimized.
This comment has been minimized.
meskill
force-pushed
the
push-xnlwnowurxsn
branch
5 times, most recently
from
f419239 to
c3e0166
Compare
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
sgrif
left a comment
sgrif
left a comment
There was a problem hiding this comment.
Would like to see more context given about why these changes make the tests less flaky. For the Ruby tests in particular it seems like this pretty fundamentally changes what they're testing, instead of testing the same thing in a more reproducible manner. If we're okay with that change it'd be nice to have more context as to why we're okay with that.
| 'notifications', (random() > 0.5) | ||
| ) AS settings | ||
| FROM generate_series(1, 10000) AS gs(id); | ||
| FROM generate_series(1, 100000) AS gs(id); |
There was a problem hiding this comment.
It's not clear from the commit context why increasing the users by 10x makes this test less flaky. Can you add that to the commit message?
| ensure | ||
| conn.close rescue nil | ||
| end | ||
| conn = connect |
There was a problem hiding this comment.
This seems like it pretty fundamentally changes the semantics of the test. Do we not care about the behavior of multiple competing threads? Can you clarify why we don't care in your commit message or PR description?
There was a problem hiding this comment.
I've changed the tests and the pr description to address this and other comments related to prepared statements, hope this helps
| mutex.synchronize { errors << e.message } | ||
| ensure | ||
| conn.close rescue nil | ||
| conn = connect |
There was a problem hiding this comment.
Same issue as the previous comment here. Can you clarify why we no longer care about the behavior with multiple competing threads?
| mutex.synchronize { errors << e.message } | ||
| ensure | ||
| conn.close rescue nil | ||
| conn = connect |
meskill
force-pushed
the
push-xnlwnowurxsn
branch
from
c3e0166 to
bf71845
Compare
meskill
force-pushed
the
push-xnlwnowurxsn
branch
from
82a72f7 to
5b499a4
Compare
Use pinned transaction instead of flaky multithreaded query execution to test the behaviour of prepared_transaction option and make sure the test request is landing on the another backend connection that hadn't prepared the transaction.
Instead of relying on a single query after preparing statement for the verification use combined approach that more reliably verifies the expected behaviour. - hold one connection with prepared_statement to verify how other pool connections are affected - run multiple parallel connection to randomize the actually used pool connection and do not rely on LIFO ordering for fetching backend connections from pool
meskill
force-pushed
the
push-xnlwnowurxsn
branch
from
5b499a4 to
a23a714
Compare
meskill
changed the title
levkk
left a comment
levkk
left a comment
There was a problem hiding this comment.
Testing a negative is hard for sure. Good job!
3 participants
fixes #1006
Improve reliability of prepared statement test by following this structure for testing inside the transaction pooling mode:
These tests should basically verify the behavior of the prepared_statement options and to do this we need to use different backend connection to verify pgdog properly proxies these calls. There is no mechanism to control it from the client so the test might look clunky but the intention is to test multiple pool connection that they behave as expected when different clients use prepared statements