Prevent app crash when postgres prepared query keys become out of sync with Rails #41356
Conversation
|
Can you squash the last three commits in one? Also can you please rebase? |
MGPalmer
force-pushed
the
fix_postgres_prepared_statement
branch
from
a6e240c
to
c727fe4
Compare
@rafaelfranca ah yes my bad, done. |
…he query. If the next query or the prepared statement itself get interrupted, this prevents the database session getting stuck perpetually retrying to recreate the same prepared statement.
MGPalmer
force-pushed
the
fix_postgres_prepared_statement
branch
from
c727fe4
to
2416da9
Compare
|
\o/ |
|
@rafaelfranca can you explain why this PR does not interfere with your argument as stated here #25827 (comment)
I feel really sad that my efforts in the linked PR were ignored for 5 years even with lots of supporters, for what was in my opinion petty bikeshedding, yet this PR here that does essentially the same thing but differently, got the go ahead really quickly. This decision feels to me like it was politically motivated more than anything. It certainly does not encourage me to contribute to Rails any longer. |
Yes. This PR had a different perspective about the problem and which cases the system could be failing. Before the argument was about rack-timeout and Now it was about
Compared with the timeframe of your PR, yes, it is true that it was quick, but I was considering this change since #41034, in the beginning still skeptical about it.
Before your PR, #17607 was opened and rejected. If I merged your PR instead #17607 would that be a politically motived decision? New arguments are introduced to discussions, the code evolve, we evolve, our decision evolve. I'm sorry that you feel that way, it wasn't my intention to discourage you from contributing to the framework |
I still don't understand the hostile environment argument, how is making a timeout not break an entire app accounting for a hostile environment? If you have code that prevents all timeouts please share it with the rails community. How is one exception raised in the context of a single request breaking an entire application acceptable? By your logic we should just have a rack timeout The fact that you can DDoS a rails app by making requests hang and timeout until the prepared statement issue happens seems like a security issue IMO I think it's stubborn and short sighted to skip over a simple and elegant solution from @dv |
Summary
Increment @counter of prepared postgres statements prior to running the query, so if the query gets interrupted without Rails' knowledge we don't end up with a crashed app in a state of perpetual failure to prepare the next query.
Other information
Based on this PR #41034 , thanks wbharding, I only streamlined the change and added a test.
Relates to #25827 . At my current project, we got hit pretty hard by the "prepared statement "ax" already exists" error and while I understand the reluctance to "fix" this in Rails, and we've already taken measures to stop the error at the root, I feel like not allowing connections to get completely stuck in a perpetual error is a good move in any case.
Any suggestions on improving the test welcome! My first time digging into the Rails (test-)code base.