From 14d3789805d22fccabcc3b9fd6ac35a3c35a6ebd Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Thu, 8 May 2025 19:49:10 +0900 Subject: [PATCH] Fix long standing bind bug with query cache. When a named statement is prepared, it is possible to bind then execute without a parse message. Problem is, table oids which are necessary to invalidate query cache at execute or COMMIT was collected only in parse messages process (Parse()). Thus if bind is executed without parse after previous execute, no table oids were collected, and pgpool failed to invalidate query cache. Fix is collecting table oids at bind time too. Add regression test to 006.memqcache. Problem reported by and test program provided by Achilleas Mantzios . Discussion: [pgpool-general: 9427] Clarification on query results cache visibility https://www.pgpool.net/pipermail/pgpool-general/2025-April/009430.html Backpatch-through: v4.2 --- src/protocol/pool_proto_modules.c | 27 ++++++++++ .../regression/tests/006.memqcache/expected.6 | 38 +++++++++++++ .../regression/tests/006.memqcache/expected.7 | 54 +++++++++++++++++++ .../tests/006.memqcache/query_cache_bug6.data | 38 +++++++++++++ .../tests/006.memqcache/query_cache_bug7.data | 52 ++++++++++++++++++ .../regression/tests/006.memqcache/test.sh | 7 ++- 6 files changed, 215 insertions(+), 1 deletion(-) create mode 100644 src/test/regression/tests/006.memqcache/expected.6 create mode 100644 src/test/regression/tests/006.memqcache/expected.7 create mode 100644 src/test/regression/tests/006.memqcache/query_cache_bug6.data create mode 100644 src/test/regression/tests/006.memqcache/query_cache_bug7.data diff --git a/src/protocol/pool_proto_modules.c b/src/protocol/pool_proto_modules.c index 6185b605c..393292487 100644 --- a/src/protocol/pool_proto_modules.c +++ b/src/protocol/pool_proto_modules.c @@ -1731,6 +1731,33 @@ Bind(POOL_CONNECTION * frontend, POOL_CONNECTION_POOL * backend, query_context->parse_tree); } + /* + * If query cache is enabled and we updating table, save table oids. It + * maybe possible that Parse() already collected the information but if + * Bind is called without prior parse (this could happen if named prepared + * statement is used), we miss the information. So we re-collect the + * information here. This could result in duplicate oids in oid table, but + * it's Okay. The duplication will be ignored anyway. + */ + if (pool_config->memory_cache_enabled) + { + /* + * Save table oids if we are updating the table. + */ + if (!query_context->is_parse_error) + { + int num_oids; + int *oids; + int i; + + num_oids = pool_extract_table_oids(query_context->parse_tree, + &oids); + /* Save to oid buffer */ + for (i = 0; i < num_oids; i++) + pool_add_dml_table_oid(oids[i]); + } + } + /* * Start a transaction if necessary in replication mode */ diff --git a/src/test/regression/tests/006.memqcache/expected.6 b/src/test/regression/tests/006.memqcache/expected.6 new file mode 100644 index 000000000..83440bba6 --- /dev/null +++ b/src/test/regression/tests/006.memqcache/expected.6 @@ -0,0 +1,38 @@ +FE=> Query (query="CREATE TABLE regress_test(i int)") +<= BE CommandComplete(CREATE TABLE) +<= BE ReadyForQuery(I) +FE=> Parse(stmt="S1", query="INSERT INTO regress_test VALUES(1)") +FE=> Bind(stmt="S1", portal="") +FE=> Execute(portal="") +FE=> Sync +<= BE ParseComplete +<= BE BindComplete +<= BE CommandComplete(INSERT 0 1) +<= BE ReadyForQuery(I) +FE=> Parse(stmt="S2", query="SELECT * FROM regress_test WHERE i = 1") +FE=> Bind(stmt="S2", portal="") +FE=> Execute(portal="") +FE=> Sync +<= BE ParseComplete +<= BE BindComplete +<= BE DataRow +<= BE CommandComplete(SELECT 1) +<= BE ReadyForQuery(I) +FE=> Bind(stmt="S1", portal="") +FE=> Execute(portal="") +FE=> Sync +<= BE BindComplete +<= BE CommandComplete(INSERT 0 1) +<= BE ReadyForQuery(I) +FE=> Bind(stmt="S2", portal="") +FE=> Execute(portal="") +FE=> Sync +<= BE BindComplete +<= BE DataRow +<= BE DataRow +<= BE CommandComplete(SELECT 2) +<= BE ReadyForQuery(I) +FE=> Query (query="DROP TABLE regress_test") +<= BE CommandComplete(DROP TABLE) +<= BE ReadyForQuery(I) +FE=> Terminate diff --git a/src/test/regression/tests/006.memqcache/expected.7 b/src/test/regression/tests/006.memqcache/expected.7 new file mode 100644 index 000000000..7304ce2ba --- /dev/null +++ b/src/test/regression/tests/006.memqcache/expected.7 @@ -0,0 +1,54 @@ +FE=> Query (query="CREATE TABLE regress_test(i int)") +<= BE CommandComplete(CREATE TABLE) +<= BE ReadyForQuery(I) +FE=> Parse(stmt="S1", query="INSERT INTO regress_test VALUES(1)") +FE=> Bind(stmt="S1", portal="") +FE=> Execute(portal="") +FE=> Sync +<= BE ParseComplete +<= BE BindComplete +<= BE CommandComplete(INSERT 0 1) +<= BE ReadyForQuery(I) +FE=> Parse(stmt="S2", query="SELECT * FROM regress_test WHERE i = 1") +FE=> Bind(stmt="S2", portal="") +FE=> Execute(portal="") +FE=> Sync +<= BE ParseComplete +<= BE BindComplete +<= BE DataRow +<= BE CommandComplete(SELECT 1) +<= BE ReadyForQuery(I) +FE=> Parse(stmt="S3", query="BEGIN") +FE=> Bind(stmt="S3", portal="") +FE=> Execute(portal="") +FE=> Sync +<= BE ParseComplete +<= BE BindComplete +<= BE CommandComplete(BEGIN) +<= BE ReadyForQuery(T) +FE=> Bind(stmt="S1", portal="") +FE=> Execute(portal="") +FE=> Sync +<= BE BindComplete +<= BE CommandComplete(INSERT 0 1) +<= BE ReadyForQuery(T) +FE=> Parse(stmt="S4", query="COMMIT") +FE=> Bind(stmt="S4", portal="") +FE=> Execute(portal="") +FE=> Sync +<= BE ParseComplete +<= BE BindComplete +<= BE CommandComplete(COMMIT) +<= BE ReadyForQuery(I) +FE=> Bind(stmt="S2", portal="") +FE=> Execute(portal="") +FE=> Sync +<= BE BindComplete +<= BE DataRow +<= BE DataRow +<= BE CommandComplete(SELECT 2) +<= BE ReadyForQuery(I) +FE=> Query (query="DROP TABLE regress_test") +<= BE CommandComplete(DROP TABLE) +<= BE ReadyForQuery(I) +FE=> Terminate diff --git a/src/test/regression/tests/006.memqcache/query_cache_bug6.data b/src/test/regression/tests/006.memqcache/query_cache_bug6.data new file mode 100644 index 000000000..360c81aba --- /dev/null +++ b/src/test/regression/tests/006.memqcache/query_cache_bug6.data @@ -0,0 +1,38 @@ +# Test case for query cache invalidation bug. +# + +# create a test table "regress_test". +'Q' "CREATE TABLE regress_test(i int)" +'Y' + +# create named statement S1 for INSERT. +'P' "S1" "INSERT INTO regress_test VALUES(1)" 0 +'B' "" "S1" 0 0 0 +'E' "" 0 +'S' +'Y' + +# create query cache. +'P' "S2" "SELECT * FROM regress_test WHERE i = 1" 0 +'B' "" "S2" 0 0 0 +'E' "" 0 +'S' +'Y' + +# INSERT one more row without bind message. +'B' "" "S1" 0 0 0 +'E' "" 0 +'S' +'Y' + +# This should return 2 rows if query cache validation succeeds. +'B' "" "S2" 0 0 0 +'E' "" 0 +'S' +'Y' + +# drop the test table. +'Q' "DROP TABLE regress_test" +'Y' + +'X' diff --git a/src/test/regression/tests/006.memqcache/query_cache_bug7.data b/src/test/regression/tests/006.memqcache/query_cache_bug7.data new file mode 100644 index 000000000..01f31ba68 --- /dev/null +++ b/src/test/regression/tests/006.memqcache/query_cache_bug7.data @@ -0,0 +1,52 @@ +# Test case for query cache invalidation bug with explicit +# transaction. + +# create a test table "regress_test". +'Q' "CREATE TABLE regress_test(i int)" +'Y' + +# create named statement S1 for INSERT. +'P' "S1" "INSERT INTO regress_test VALUES(1)" 0 +'B' "" "S1" 0 0 0 +'E' "" 1 +'S' +'Y' + +# create query cache. +'P' "S2" "SELECT * FROM regress_test WHERE i = 1" 0 +'B' "" "S2" 0 0 0 +'E' "" 0 +'S' +'Y' + +# start an explicit transaction. +'P' "S3" "BEGIN" 0 +'B' "" "S3" 0 0 0 +'E' "" 0 +'S' +'Y' + +# INSERT one more row without bind message. +'B' "" "S1" 0 0 0 +'E' "" 1 +'S' +'Y' + +# commit the transaction. +'P' "S4" "COMMIT" 0 +'B' "" "S4" 0 0 0 +'E' "" 0 +'S' +'Y' + +# This should return 2 rows if query cache validation succeeds. +'B' "" "S2" 0 0 0 +'E' "" 0 +'S' +'Y' + +# drop the test table. +'Q' "DROP TABLE regress_test" +'Y' + +'X' diff --git a/src/test/regression/tests/006.memqcache/test.sh b/src/test/regression/tests/006.memqcache/test.sh index f133f415a..335f14df4 100755 --- a/src/test/regression/tests/006.memqcache/test.sh +++ b/src/test/regression/tests/006.memqcache/test.sh @@ -488,7 +488,7 @@ echo "done." echo "memory_cache_enabled = on" >> etc/pgpool.conf cd .. -for i in 1 2 3 4 4 5 +for i in 1 2 3 4 4 5 6 7 do # # case 1: failed with kind mismatch error at #5. @@ -503,6 +503,11 @@ do # the test works for either query cache exists or does not exist # # case 5: simple cache invalidation test. + # + # case 6: execute INSERT without parse message cache invalidation test. + # + # case 7: similar to case 6 except this uses an explicit transaction. + cd $TESTDIR # case 5 includes UPDATE, and we want the result without disturbed -- 2.39.5