From: Tatsuo Ishii Date: Thu, 1 May 2025 23:35:33 +0000 (+0900) Subject: Fix query cache invalidation bug. X-Git-Url: http://git.postgresql.org/gitweb/?a=commitdiff_plain;h=3f36fe3a39fe7c9cceec368d465506c7d1bc1c27;p=pgpool2.git Fix query cache invalidation bug. When an execute message is received, pgpool checks its max number of rows paramter. If it's not zero, pgpool sets "partial_fetch" flag to instruct pool_handle_query_cache() to not create query cache. Problem is, commit 2a99aa5d1 missed that even INSERT/UPDATE/DELETE sets the execute message parameter to non 0 (mostly 1) and pgpool set the flag for even none SELECTs. This resulted in failing to invalidate query cache because if the flag is true, subsequent code in pool_handle_query_cache() skips cache invalidation. It was an oversight in this commit (my fault): https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=2a99aa5d1910f1fd4855c8eb6751a26cbaa5e48d To fix this change Execute() to check if the query is read only SELECT before setting the flag. Also add test to 006.memqcache. Problem reported by and a 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 59647fe81..5b3e03d59 100644 --- a/src/protocol/pool_proto_modules.c +++ b/src/protocol/pool_proto_modules.c @@ -3,7 +3,7 @@ * pgpool: a language independent connection pool server for PostgreSQL * written by Tatsuo Ishii * - * Copyright (c) 2003-2024 PgPool Global Development Group + * Copyright (c) 2003-2025 PgPool Global Development Group * * Permission to use, copy, modify, and distribute this software and * its documentation for any purpose and without fee is hereby @@ -950,6 +950,9 @@ Execute(POOL_CONNECTION * frontend, POOL_CONNECTION_POOL * backend, /* obtain number of returning rows */ p = contents + strlen(contents) + 1; memcpy(&num_rows, p, sizeof(num_rows)); + num_rows = ntohl(num_rows); + ereport(DEBUG1, + (errmsg("Execute: num_rows: %d", num_rows))); bind_msg = pool_get_sent_message('B', contents, POOL_SENT_MESSAGE_CREATED); if (!bind_msg) @@ -981,11 +984,12 @@ Execute(POOL_CONNECTION * frontend, POOL_CONNECTION_POOL * backend, query = bind_msg->query_context->original_query; /* - * If execute message's parameter is not 0, set partial_fetch flag to true - * so that subsequent execute message knows that the portal started with - * partial fetching. + * If execute message's parameter is not 0 and the query is cache safe + * (i.e. read only SELECT), set partial_fetch flag to true so that + * subsequent execute message knows that the portal started with partial + * fetching. */ - if (num_rows != 0) + if (num_rows != 0 && query_context->is_cache_safe && !query_cache_disabled()) { query_context->partial_fetch = true; elog(DEBUG1, "set partial_fetch in execute"); diff --git a/src/test/regression/tests/006.memqcache/expected.5 b/src/test/regression/tests/006.memqcache/expected.5 new file mode 100644 index 000000000..cd425f0e6 --- /dev/null +++ b/src/test/regression/tests/006.memqcache/expected.5 @@ -0,0 +1,37 @@ +FE=> Query (query="CREATE TABLE regress_test(i int)") +<= BE CommandComplete(CREATE TABLE) +<= BE ReadyForQuery(I) +FE=> Query (query="INSERT INTO regress_test VALUES(1)") +<= BE CommandComplete(INSERT 0 1) +<= BE ReadyForQuery(I) +FE=> Parse(stmt="S0", query="SELECT * FROM regress_test WHERE i = 1") +FE=> Bind(stmt="S0", portal="P0") +FE=> Describe(portal="P0") +FE=> Execute(portal="P0") +FE=> Sync +<= BE ParseComplete +<= BE BindComplete +<= BE RowDescription +<= BE DataRow +<= BE CommandComplete(SELECT 1) +<= BE ReadyForQuery(I) +FE=> Parse(stmt="S2", query="UPDATE regress_test SET i = 2") +FE=> Bind(stmt="S2", portal="P2") +FE=> Execute(portal="P2") +FE=> Sync +<= BE ParseComplete +<= BE BindComplete +<= BE CommandComplete(UPDATE 1) +<= BE ReadyForQuery(I) +FE=> Bind(stmt="S0", portal="P0") +FE=> Describe(portal="P0") +FE=> Execute(portal="P0") +FE=> Sync +<= BE BindComplete +<= BE RowDescription +<= BE CommandComplete(SELECT 0) +<= 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_bug5.data b/src/test/regression/tests/006.memqcache/query_cache_bug5.data new file mode 100644 index 000000000..f013f11a1 --- /dev/null +++ b/src/test/regression/tests/006.memqcache/query_cache_bug5.data @@ -0,0 +1,37 @@ +# Execute UPDATE to check if the query cache is invalidated. + +# create a test table "regress_test". +'Q' "CREATE TABLE regress_test(i int)" +'Y' +'Q' "INSERT INTO regress_test VALUES(1)" +'Y' + +'P' "S0" "SELECT * FROM regress_test WHERE i = 1" 0 +'B' "P0" "S0" 0 0 0 +'D' 'P' "P0" +# This SELECT is expected to return 1 row and it should have created +# query cache. +'E' "P0" 0 +'S' +'Y' + +'P' "S2" "UPDATE regress_test SET i = 2" 0 +'B' "P2" "S2" 0 0 0 +# Set "maxrows" parameter to 1 of this execute message to trigger bug. +'E' "P2" 1 +'S' +'Y' + +'B' "P0" "S0" 0 0 0 +'D' 'P' "P0" +# This SELECT is expected to return 0 row because previous UPDATE +# should have deleted the query cache. +'E' "P0" 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 04f1ba131..b674fdf70 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 +for i in 1 2 3 4 4 5 do # # case 1: failed with kind mismatch error at #5. @@ -553,7 +553,15 @@ do # case 4: various cases including portal suspended # Note that case4 is executed twice to make sure that # the test works for either query cache exists or does not exist + # + # case 5: simple cache invalidation test. cd $TESTDIR + + # case 5 includes UPDATE, and we want the result without disturbed + # by replication delay. + if [ $i = 5 ];then + echo "backend_weight1 = 0" >> etc/pgpool.conf + fi ./startall wait_for_pgpool_startup timeout 1 $PGPROTO -d test -f ../query_cache_bug$i.data |& del_details_from_error > result