From: Tatsuo Ishii Date: Thu, 8 May 2025 10:49:10 +0000 (+0900) Subject: Fix long standing bind bug with query cache. X-Git-Tag: V4_7_0_BETA1~127 X-Git-Url: http://waps.l3s.uni-hannover.de/gitweb/faq.html?a=commitdiff_plain;h=35a5a79d623b0ae7023437ac0360fcb2f6300d19;p=pgpool2.git 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 --- diff --git a/src/protocol/pool_proto_modules.c b/src/protocol/pool_proto_modules.c index 5b3e03d59..d79357211 100644 --- a/src/protocol/pool_proto_modules.c +++ b/src/protocol/pool_proto_modules.c @@ -1779,6 +1779,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 b674fdf70..d00bdd084 100755 --- a/src/test/regression/tests/006.memqcache/test.sh +++ b/src/test/regression/tests/006.memqcache/test.sh @@ -540,7 +540,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. @@ -555,6 +555,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