From 70a2ac32c5ff5f72077a928bc88563ee1b0ea34b Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Tue, 14 Jul 2020 22:07:59 +0900 Subject: [PATCH] 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 --- src/include/query_cache/pool_memqcache.h | 1 + src/protocol/pool_proto_modules.c | 2 +- src/query_cache/pool_memqcache.c | 127 +++++++++++++++++- .../regression/tests/006.memqcache/test.sh | 6 + src/utils/pool_select_walker.c | 6 + 5 files changed, 140 insertions(+), 2 deletions(-) diff --git a/src/include/query_cache/pool_memqcache.h b/src/include/query_cache/pool_memqcache.h index ee0d23bac..e5e7e6eba 100644 --- a/src/include/query_cache/pool_memqcache.h +++ b/src/include/query_cache/pool_memqcache.h @@ -242,6 +242,7 @@ 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_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 65d0c0c29..7d95cbbb3 100644 --- a/src/protocol/pool_proto_modules.c +++ b/src/protocol/pool_proto_modules.c @@ -465,7 +465,7 @@ SimpleQuery(POOL_CONNECTION * frontend, * 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 (!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 84337ddfc..5a3d91b44 100644 --- a/src/query_cache/pool_memqcache.c +++ b/src/query_cache/pool_memqcache.c @@ -925,6 +925,27 @@ 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; } @@ -963,6 +984,8 @@ 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. @@ -990,21 +1013,29 @@ pool_extract_table_oids(Node *node, int **oidsp) { 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; + 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; + 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 /* @@ -1129,6 +1160,73 @@ 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; @@ -3404,14 +3502,41 @@ pool_handle_query_cache(POOL_CONNECTION_POOL * backend, char *query, Node *node, /* 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 02566d338..0272cf4e3 100755 --- 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 < /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 f482c1c28..bd5362166 100644 --- a/src/utils/pool_select_walker.c +++ b/src/utils/pool_select_walker.c @@ -1247,6 +1247,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); } -- 2.39.5