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
<[email protected]>.

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 03c817449a4b81c7c05814ada46e495daa12f544..80cf5b0fc176e87dd2037d4a2c31196a989a61b9 100644 (file)
--- a/src/protocol/pool_proto_modules.c
+++ b/src/protocol/pool_proto_modules.c
@@ -1746,6 +1746,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 (file)
index 0000000..83440bb
--- /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 (file)
index 0000000..7304ce2
--- /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 (file)
index 0000000..360c81a
--- /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 (file)
index 0000000..01f31ba
--- /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 f133f415a8e4cd1f173809ea852fe3eff37c81b5..335f14df4207a71cab5a150bd3abe065b38774a0 100755 (executable)
--- 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