Prevent data modifying CTE to be cached.

Data modifying CTE was mistakenly treated as normal read only CTE and
result query was created.  As a result subsequent CTE was not
executed.

Problem reported and patch created by Hou, Zhijie.
Subtle changes to the regression test by me.
Discussion: https://www.pgpool.net/pipermail/pgpool-hackers/2020-July/003705.html

diff --git a/src/include/query_cache/pool_memqcache.h b/src/include/query_cache/pool_memqcache.h
index 63e1e71ab3bf5d67e5e80051ea0a01d4b494917d..9c0e1a3fb6f03ffe7e1c37329539b90f8aa8f328 100644 (file)
--- a/src/include/query_cache/pool_memqcache.h
+++ b/src/include/query_cache/pool_memqcache.h
@@ -229,7 +229,8 @@ extern bool pool_is_likely_select(char *query);
 extern bool pool_is_table_in_black_list(const char *table_name);
 extern bool pool_is_table_in_white_list(const char *table_name);
 extern bool pool_is_allow_to_cache(Node *node, char *query);
-extern int pool_extract_table_oids(Node *node, int **oidsp);
+extern int     pool_extract_table_oids(Node *node, int **oidsp);
+extern int     pool_extract_withclause_oids(Node *with, int *oidsp);
 extern void pool_add_dml_table_oid(int oid);
 extern void pool_discard_oid_maps(void);
 extern int pool_get_database_oid_from_dbname(char *dbname);

diff --git a/src/protocol/pool_proto_modules.c b/src/protocol/pool_proto_modules.c
index ba7cf1c7c5e0e42b8637e225cefbb4f4151653af..3330238f90cfcbc3d1a82c4bd5cd18aeb6ad5267 100644 (file)
--- a/src/protocol/pool_proto_modules.c
+++ b/src/protocol/pool_proto_modules.c
@@ -452,8 +452,11 @@ POOL_STATUS SimpleQuery(POOL_CONNECTION *frontend,
                                pool_unset_cache_safe();
                        }
 
-                       /* If table is to be cached and the query is DML, save the table oid */
-                       if (!is_select_query && !query_context->is_parse_error)
+                       /*
+                        * If table is to be cached and the query is DML, save the table
+                        * oid
+                        */
+                       if (!query_context->is_parse_error)
                        {
                                num_oids = pool_extract_table_oids(node, &oids);
 

diff --git a/src/query_cache/pool_memqcache.c b/src/query_cache/pool_memqcache.c
index b5c2e3312a1248a85cdfc011ebdc668646050e65..a3b4e223dba9a74d24951d32983393b4bfcb1d73 100644 (file)
--- a/src/query_cache/pool_memqcache.c
+++ b/src/query_cache/pool_memqcache.c
@@ -906,6 +906,27 @@ bool pool_is_allow_to_cache(Node *node, char *query)
                if (pool_has_unlogged_table(node))
                        return false;
        }
+
+       /*
+        * If Data-modifying statements in WITH clause, it's not allowed to cache.
+        */
+       if(IsA(node, SelectStmt) && ((SelectStmt *) node)->withClause)
+       {
+               ListCell        *lc;
+               WithClause      *withClause = ((SelectStmt *) node)->withClause;
+
+               foreach(lc, withClause->ctes)
+               {
+                       CommonTableExpr *cte = (CommonTableExpr *)lfirst(lc);
+                       if(IsA(cte->ctequery, InsertStmt) ||
+                          IsA(cte->ctequery, DeleteStmt) ||
+                          IsA(cte->ctequery, UpdateStmt))
+                       {
+                               return false;
+                       }
+               }
+       }
+
        return true;
 }
 
@@ -942,6 +963,8 @@ bool pool_is_table_in_white_list(const char *table_name)
 /*
  * Extract table oid from INSERT/UPDATE/DELETE/TRUNCATE/
  * DROP TABLE/ALTER TABLE/COPY FROM statement.
+ * For SELECT, if Data-modifying statements in its WITH clause,
+ * extract table oid from Data-modifying statements too.
  * Returns number of oids.
  * In case of error, returns 0 (InvalidOid).
  * oids buffer (oidsp) will be discarded by subsequent call.
@@ -966,20 +989,31 @@ int pool_extract_table_oids(Node *node, int **oidsp)
 
        if (IsA(node, InsertStmt))
        {
-               InsertStmt *stmt = (InsertStmt *)node;
+               InsertStmt *stmt = (InsertStmt *) node;
+
+               num_oids = pool_extract_withclause_oids((Node *) stmt->withClause, *oidsp);
                table = make_table_name_from_rangevar(stmt->relation);
        }
        else if (IsA(node, UpdateStmt))
        {
-               UpdateStmt *stmt = (UpdateStmt *)node;
+               UpdateStmt *stmt = (UpdateStmt *) node;
+
+               num_oids = pool_extract_withclause_oids((Node *) stmt->withClause, *oidsp);
                table = make_table_name_from_rangevar(stmt->relation);
        }
        else if (IsA(node, DeleteStmt))
        {
-               DeleteStmt *stmt = (DeleteStmt *)node;
+               DeleteStmt *stmt = (DeleteStmt *) node;
+
+               num_oids = pool_extract_withclause_oids((Node *) stmt->withClause, *oidsp);
                table = make_table_name_from_rangevar(stmt->relation);
        }
-
+       else if(IsA(node, SelectStmt))
+       {
+               SelectStmt *stmt = (SelectStmt *) node;
+               num_oids = pool_extract_withclause_oids((Node *) stmt->withClause, *oidsp);
+               table = NULL;
+       }
 #ifdef NOT_USED
        /*
         * We do not handle CREATE TABLE here.  It is possible that
@@ -1100,6 +1134,73 @@ int pool_extract_table_oids(Node *node, int **oidsp)
        return num_oids;
 }
 
+/*
+ * Extract table oid from INSERT/UPDATE/DELETE
+ * FROM statement in WITH clause.
+ * Returns number of oids.
+ * oids buffer (oidsp) will be discarded by subsequent call.
+ */
+int
+pool_extract_withclause_oids(Node *node, int *oidsp)
+{
+       int                     num_oids = 0;
+       int                     oid;
+       char       *table;
+       ListCell   *lc;
+       WithClause *with;
+
+       if(oidsp == NULL)
+       {
+               return 0;
+       }
+
+       if(!node || !IsA(node, WithClause))
+       {
+               return 0;
+       }
+
+       with = (WithClause *) node;
+       foreach(lc, with->ctes)
+       {
+               CommonTableExpr *cte = (CommonTableExpr *)lfirst(lc);
+               if(IsA(cte->ctequery, InsertStmt))
+               {
+                       InsertStmt *stmt = (InsertStmt *) cte->ctequery;
+                       table = make_table_name_from_rangevar(stmt->relation);
+               }
+               else if(IsA(cte->ctequery, DeleteStmt))
+               {
+                       DeleteStmt *stmt = (DeleteStmt *) cte->ctequery;
+                       table = make_table_name_from_rangevar(stmt->relation);
+               }
+               else if(IsA(cte->ctequery, UpdateStmt))
+               {
+                       UpdateStmt *stmt = (UpdateStmt *) cte->ctequery;
+                       table = make_table_name_from_rangevar(stmt->relation);
+               }
+               else
+               {
+                       /* only check INSERT/DELETE/UPDATE in WITH clause */
+                       table = NULL;
+               }
+
+               oid = pool_table_name_to_oid(table);
+               if (oid > 0)
+               {
+                       if (num_oids >= POOL_MAX_DML_OIDS)
+                       {
+                               break;
+                       }
+
+                       oidsp[num_oids++] = pool_table_name_to_oid(table);
+                       ereport(DEBUG1,
+                                       (errmsg("memcache: extracting table oids: table: \"%s\" oid:%d", table, oidsp[num_oids - 1])));
+               }
+       }
+
+       return num_oids;
+}
+
 #define POOL_OIDBUF_SIZE 1024
 static int* oidbuf;
 static int oidbufp;
@@ -3309,14 +3410,41 @@ void pool_handle_query_cache(POOL_CONNECTION_POOL *backend, char *query, Node *n
                /* Non cachable SELECT */
                if (node && IsA(node, SelectStmt))
                {
+                       /* Extract table oids from buffer */
+                       num_oids = pool_get_dml_table_oid(&oids);
+
                        if (state == 'I')
                        {
+                               /*
+                                * If Data-modifying statements in SELECT's WITH clause,
+                                * invalidate query cache.
+                                */
+                               if (num_oids > 0 && pool_config->memqcache_auto_cache_invalidation)
+                               {
+                                       POOL_SETMASK2(&BlockSig, &oldmask);
+                                       pool_shmem_lock();
+                                       pool_invalidate_query_cache(num_oids, oids, true, 0);
+                                       pool_shmem_unlock();
+                                       POOL_SETMASK(&oldmask);
+                               }
+
                                /* Count up SELECT stats */
                                pool_stats_count_up_num_selects(1);
                                pool_reset_memqcache_buffer(true);
                        }
                        else
                        {
+                               /*
+                                * If we are inside a transaction, we cannot invalidate
+                                * query cache yet. However we can clear cache buffer, if
+                                * DML/DDL modifies the TABLE which SELECT uses.
+                                */
+                               if (num_oids > 0 && pool_config->memqcache_auto_cache_invalidation)
+                               {
+                                       pool_check_and_discard_cache_buffer(num_oids, oids);
+                                       pool_reset_memqcache_buffer(false);
+                               }
+
                                /* Count up temporary SELECT stats */
                                pool_tmp_stats_count_up_num_selects();
                        }

diff --git a/src/test/regression/tests/006.memqcache/test.sh b/src/test/regression/tests/006.memqcache/test.sh
index 02566d3382fca776c3947bdd83f28ea6afcd12c9..0272cf4e3c17b255873b135691a564228f7c6a6e 100755 (executable)
--- a/src/test/regression/tests/006.memqcache/test.sh
+++ b/src/test/regression/tests/006.memqcache/test.sh
@@ -41,6 +41,7 @@ do
        $PSQL test <<EOF
 CREATE TABLE t1 (i int);
 CREATE TABLE black_t (i int);
+CREATE TABLE with_modify (i int);
 CREATE VIEW normal_v AS SELECT * FROM t1;
 CREATE VIEW white_v AS SELECT * FROM t1;
 SELECT pg_sleep(2);    -- Sleep for a while to make sure object creations are replicated
@@ -52,6 +53,10 @@ SELECT * FROM normal_v;
 SELECT * FROM normal_v;
 SELECT * FROM white_v;
 SELECT * FROM white_v;
+SELECT * FROM with_modify;
+WITH cte AS (INSERT INTO with_modify values(1) RETURNING *) SELECT * FROM with_modify;
+WITH cte AS (INSERT INTO with_modify values(1) RETURNING *) SELECT * FROM with_modify;
+SELECT * FROM with_modify;
 EOF
 
        success=true
@@ -59,6 +64,7 @@ EOF
        grep "fetched from cache" log/pgpool.log | grep black_t > /dev/null && success=false
        grep "fetched from cache" log/pgpool.log | grep normal_v > /dev/null && success=false
        grep "fetched from cache" log/pgpool.log | grep white_v > /dev/null || success=false
+       grep "fetched from cache" log/pgpool.log | grep with_modify > /dev/null && success=false
        if [ $success = false ];then
                ./shutdownall
                exit 1

diff --git a/src/utils/pool_select_walker.c b/src/utils/pool_select_walker.c
index 89d7a98fd6c2760352d47f3ad8218d94a02d33d1..29fd05a9387d7ba1eb4f2b04534c7b3c1ff13fbe 100644 (file)
--- a/src/utils/pool_select_walker.c
+++ b/src/utils/pool_select_walker.c
@@ -1200,6 +1200,12 @@ select_table_walker(Node *node, void *context)
                }
        }
 
+       /* Skip Data-Modifying Statements in SELECT. */
+       else if (IsA(node, InsertStmt) || IsA(node, DeleteStmt) || IsA(node, UpdateStmt))
+       {
+               return false;
+       }
+
        return raw_expression_tree_walker(node, select_table_walker, context);
 }