From c08cc7230677367d00a797e6f6120909fe05134b Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 23 Sep 2023 10:35:42 -0500 Subject: [PATCH] Improve QueryExecModeCacheDescribe and clarify documentation QueryExecModeCacheDescribe actually is safe even when the schema or search_path is modified. It may return an error on the first execution but it should never silently encode or decode a value incorrectly. Add a test to demonstrate and ensure this behavior. Update documentation of QueryExecModeCacheDescribe to remove warning of undetected result decoding errors. Update documentation of QueryExecModeCacheStatement and QueryExecModeCacheDescribe to indicate that the first execution of an invalidated statement may fail. --- conn.go | 13 +++++--- internal/stmtcache/stmtcache.go | 16 --------- query_test.go | 58 +++++++++++++++++++++++++++++++++ rows.go | 13 +++----- 4 files changed, 71 insertions(+), 29 deletions(-) diff --git a/conn.go b/conn.go index a15c280e4..7d5b79d05 100644 --- a/conn.go +++ b/conn.go @@ -598,13 +598,16 @@ type QueryExecMode int32 const ( _ QueryExecMode = iota - // Automatically prepare and cache statements. This uses the extended protocol. Queries are executed in a single - // round trip after the statement is cached. This is the default. + // Automatically prepare and cache statements. This uses the extended protocol. Queries are executed in a single round + // trip after the statement is cached. This is the default. If the database schema is modified or the search_path is + // changed after a statement is cached then the first execution of a previously cached query may fail. e.g. If the + // number of columns returned by a "SELECT *" changes or the type of a column is changed. QueryExecModeCacheStatement - // Cache statement descriptions (i.e. argument and result types) and assume they do not change. This uses the - // extended protocol. Queries are executed in a single round trip after the description is cached. If the database - // schema is modified or the search_path is changed this may result in undetected result decoding errors. + // Cache statement descriptions (i.e. argument and result types) and assume they do not change. This uses the extended + // protocol. Queries are executed in a single round trip after the description is cached. If the database schema is + // modified or the search_path is changed after a statement is cached then the first execution of a previously cached + // query may fail. e.g. If the number of columns returned by a "SELECT *" changes or the type of a column is changed. QueryExecModeCacheDescribe // Get the statement description on every execution. This uses the extended protocol. Queries require two round trips diff --git a/internal/stmtcache/stmtcache.go b/internal/stmtcache/stmtcache.go index a86755cc4..b2940e230 100644 --- a/internal/stmtcache/stmtcache.go +++ b/internal/stmtcache/stmtcache.go @@ -38,19 +38,3 @@ type Cache interface { // Cap returns the maximum number of cached prepared statement descriptions. Cap() int } - -func IsStatementInvalid(err error) bool { - pgErr, ok := err.(*pgconn.PgError) - if !ok { - return false - } - - // https://github.com/jackc/pgx/issues/1162 - // - // We used to look for the message "cached plan must not change result type". However, that message can be localized. - // Unfortunately, error code "0A000" - "FEATURE NOT SUPPORTED" is used for many different errors and the only way to - // tell the difference is by the message. But all that happens is we clear a statement that we otherwise wouldn't - // have so it should be safe. - possibleInvalidCachedPlanError := pgErr.Code == "0A000" - return possibleInvalidCachedPlanError -} diff --git a/query_test.go b/query_test.go index 6d7e91dfe..92f431c78 100644 --- a/query_test.go +++ b/query_test.go @@ -1928,6 +1928,64 @@ func TestQueryErrorWithDisabledStatementCache(t *testing.T) { ensureConnValid(t, conn) } +func TestConnQueryQueryExecModeCacheDescribeSafeEvenWhenTypesChange(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + defer cancel() + + conn := mustConnectString(t, os.Getenv("PGX_TEST_DATABASE")) + defer closeConn(t, conn) + + _, err := conn.Exec(ctx, `create temporary table to_change ( + name text primary key, + age int +); + +insert into to_change (name, age) values ('John', 42);`) + require.NoError(t, err) + + var name string + var ageInt32 int32 + err = conn.QueryRow(ctx, "select * from to_change where age = $1", pgx.QueryExecModeCacheDescribe, int32(42)).Scan(&name, &ageInt32) + require.NoError(t, err) + require.Equal(t, "John", name) + require.Equal(t, int32(42), ageInt32) + + _, err = conn.Exec(ctx, `alter table to_change alter column age type float4;`) + require.NoError(t, err) + + err = conn.QueryRow(ctx, "select * from to_change where age = $1", pgx.QueryExecModeCacheDescribe, int32(42)).Scan(&name, &ageInt32) + require.NoError(t, err) + require.Equal(t, "John", name) + require.Equal(t, int32(42), ageInt32) + + var ageFloat32 float32 + err = conn.QueryRow(ctx, "select * from to_change where age = $1", pgx.QueryExecModeCacheDescribe, int32(42)).Scan(&name, &ageFloat32) + require.NoError(t, err) + require.Equal(t, "John", name) + require.Equal(t, float32(42), ageFloat32) + + _, err = conn.Exec(ctx, `alter table to_change drop column name;`) + require.NoError(t, err) + + // Number of result columns has changed, so just like with a prepared statement, this will fail the first time. + err = conn.QueryRow(ctx, "select * from to_change where age = $1", pgx.QueryExecModeCacheDescribe, int32(42)).Scan(&ageFloat32) + require.EqualError(t, err, "ERROR: bind message has 2 result formats but query has 1 columns (SQLSTATE 08P01)") + + // But it will work the second time after the cache is invalidated. + err = conn.QueryRow(ctx, "select * from to_change where age = $1", pgx.QueryExecModeCacheDescribe, int32(42)).Scan(&ageFloat32) + require.NoError(t, err) + require.Equal(t, float32(42), ageFloat32) + + _, err = conn.Exec(ctx, `alter table to_change alter column age type numeric;`) + require.NoError(t, err) + + err = conn.QueryRow(ctx, "select * from to_change where age = $1", pgx.QueryExecModeCacheDescribe, int32(42)).Scan(&ageFloat32) + require.NoError(t, err) + require.Equal(t, float32(42), ageFloat32) +} + func TestQueryWithQueryRewriter(t *testing.T) { t.Parallel() diff --git a/rows.go b/rows.go index a497738a0..444a12410 100644 --- a/rows.go +++ b/rows.go @@ -8,7 +8,6 @@ import ( "strings" "time" - "github.com/jackc/pgx/v5/internal/stmtcache" "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgtype" ) @@ -174,14 +173,12 @@ func (rows *baseRows) Close() { } if rows.err != nil && rows.conn != nil && rows.sql != "" { - if stmtcache.IsStatementInvalid(rows.err) { - if sc := rows.conn.statementCache; sc != nil { - sc.Invalidate(rows.sql) - } + if sc := rows.conn.statementCache; sc != nil { + sc.Invalidate(rows.sql) + } - if sc := rows.conn.descriptionCache; sc != nil { - sc.Invalidate(rows.sql) - } + if sc := rows.conn.descriptionCache; sc != nil { + sc.Invalidate(rows.sql) } }