Deallocate a failed prepare by the name sent in Parse, and only when Parse completed - #2644
Open
eliranbz wants to merge 2 commits into
Open
Deallocate a failed prepare by the name sent in Parse, and only when Parse completed#2644eliranbz wants to merge 2 commits into
eliranbz wants to merge 2 commits into
Conversation
When Prepare is called with name == sql (the stdlib/database/sql path), the statement is prepared on the server under stmt_<digest> while the client keys it by the SQL text. The cleanup for a prepare that failed during Describe stored that client key and later deallocated by it, sending Close for a statement name that does not exist. The real stmt_<digest> leaked on the server, and any retry of the same SQL on that connection failed with 42P05 duplicate_prepared_statement for the rest of the connection's life. The cleanup's own round trip could also surface its errors on whatever query next prepared on the connection, quoting the old query's full SQL. Store the name actually sent in Parse so the deallocation closes the real statement. The named-prepare path is unchanged: there the key and the wire name are the same. jackc#2640
A PrepareError with ParseComplete == false means the server rejected the Parse itself — no statement was created, so there is nothing to deallocate. Scheduling the cleanup anyway cost a wasted round trip at the start of the next Prepare on the connection after every failed prepare (e.g. any syntax error), and gave that round trip a chance to fail and surface its error on an unrelated query. jackc#2640
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2640
database/sql (and GORM with
PrepareStmt: true) end up callingPrepare(ctx, sql, sql). In that path the server-side statement is namedstmt_<digest>but the client keys it by the SQL text. When a prepare failed during Describe, the cleanup remembered the client key and later deallocated by it, i.e. it sent Close for a name that never existed. So the realstmt_<digest>stayed on the server, retrying the same SQL on that connection hit 42P05 for the rest of its life, and the cleanup error could pop up on whatever query happened to prepare next on that pooled connection.The first commit stores
psName(what was actually sent in Parse) instead, so the Close hits the real statement. Named prepares are unaffected sincepsName == psKeythere. The test is the digested-name variant ofTestPrepareHandlesTimeoutBetweenParseAndDescribe, and like that test it checkspg_prepared_statementsdirectly for the leaked statement.While writing the test I noticed
PrepareError.ParseCompleteis never checked. If Parse itself failed (any syntax error, say) there is no statement on the server, but we would still schedule a deallocation: a wasted round trip at the start of the next Prepare, which can also fail and surface its error on an unrelated query. The second commit only schedules the cleanup when ParseComplete is true, with a test that verifies no Close message goes out in that case.I saw #2642 already has the rename half of this. Since I filed the issue I wanted to put up the complete fix, with the ParseComplete part and the leak assertion included.
Tested against PostgreSQL 17: both new tests fail before their respective commit and pass after, and the full package and stdlib suites pass.
I also reproduced the incident that led to the issue through the same application stack where we hit it in production — GORM with
PrepareStmt: trueover stdlib, single pooled connection, prepare killed bystatement_timeoutbetween Parse and Describe. On v5.10.0 the connection is poisoned: every retry of that query returns 42P05 for the connection's remaining life. On this branch the first retry succeeds and the connection recovers.