Fix EXISTS restriction

[thoshiai]

reinforce EXISTS restriction, only support the AND condition.

[yugo-n]

I copy and paste the patches here for making your review easier.

0001-Fix-check-for-EXISTS-subquery-support.patch

```diff From 7e93cc1e176fbfda1d0cfe3bf4969988197dc680 Mon Sep 17 00:00:00 2001 From: Yugo Nagata Date: Tue, 27 Feb 2024 10:38:33 +0900 Subject: [PATCH 1/2] Fix check for EXISTS subquery support Currently, EXISTS subquery is allowed only directly under WHERE clause or in AND expression that is directly under WHERE. In order to confirm this, I added a new boolean member allow_context into check_ivm_restriction_context. This member means whether EXISTS subquery is allowed in the current node being examined in check_ivm_restriction_walker. This should be set just before calling check_ivm_restriction_walker for nodes directly under WHERE or operands of AND expression direct under WHERE, and is reset to false on every call of the function. In passing, I removed unnecessary members in check_ivm_restriction_context, and moved the check for OR and NOT expression was moved from rewrite_exists_subquery_walker to check_ivm_restriction_context. --- createas.c | 105 ++++++++++++++++++++++++-------------------- expected/pg_ivm.out | 9 ++-- matview.c | 42 +++++++----------- 3 files changed, 79 insertions(+), 77 deletions(-) diff --git a/createas.c b/createas.c index 39f2807..e928023 100644 --- a/createas.c +++ b/createas.c @@ -65,14 +65,11 @@ typedef struct typedef struct { - bool has_agg; - bool has_sublinks; - bool has_subquery; - bool in_exists_subquery; /* true, if it is in a exists subquery */ - bool in_jointree; /* true, if it is in a join tree */ - bool is_simple_jointree; /* true, if it is in a simple join tree or boolexpr directly under jointree */ - List *exists_qual_vars; - int sublevels_up; + bool has_agg; /* the query has an aggregate? */ + bool allow_exists; /* is EXISTS subquery allowed in the current node? */ + bool in_exists_subquery; /* true, if under an EXISTS subquery */ + List *exists_qual_vars; /* Vars used in EXISTS subqueries */ + int sublevels_up; /* (current) nesting depth */ } check_ivm_restriction_context; static void CreateIvmTriggersOnBaseTablesRecurse(Query *qry, Node *node, Oid matviewOid, @@ -742,7 +739,13 @@ CreateIvmTrigger(Oid relOid, Oid viewOid, int16 type, int16 timing, bool ex_lock static void check_ivm_restriction(Node *node) { - check_ivm_restriction_context context = {false, false, false, false, false, false, NIL, 0}; + check_ivm_restriction_context context; + + context.has_agg = false; + context.allow_exists = false; + context.in_exists_subquery = false; + context.exists_qual_vars = NIL; + context.sublevels_up = 0;; check_ivm_restriction_walker(node, &context); } @@ -750,6 +753,10 @@ check_ivm_restriction(Node *node) static bool check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) { + /* EXISTS is allowed only in this node */ + bool allow_exists = context->allow_exists; + context->allow_exists = false; + if (node == NULL) return false; @@ -809,8 +816,6 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("recursive query is not supported on incrementally maintainable materialized view"))); - context->has_sublinks |= qry->hasSubLinks; - /* system column restrictions */ vars = pull_vars_of_level((Node *) qry, 0); foreach(lc, vars) @@ -904,10 +909,8 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) if (rte->rtekind == RTE_SUBQUERY) { - context->has_subquery = true; - context->sublevels_up++; - check_ivm_restriction_walker((Node *)rte->subquery, context); + check_ivm_restriction_walker((Node *) rte->subquery, context); context->sublevels_up--; } } @@ -997,38 +1000,17 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) expression_tree_walker(node, check_ivm_restriction_walker, (void *) context); break; } - case T_List: + case T_FromExpr: { - ListCell *temp; - foreach(temp, (List *) node) - { - check_ivm_restriction_walker(lfirst(temp), context); - } - break; - } - case T_FromExpr: - { - FromExpr *from = (FromExpr *) node; + FromExpr *from = (FromExpr *) node; + + check_ivm_restriction_walker((Node *) from->fromlist, context); - check_ivm_restriction_walker((Node *)from->fromlist, context); /* - * check the sublink restrictions. - * Currently, EXISTS subqueries with condition other than 'AND' is not supported. + * EXIEST is allowed directly under FROM clause */ - if (from->quals == NULL) - break; - if (context->has_sublinks && !context->in_exists_subquery && - !IsA(from->quals, SubLink) && !IsA(from->quals, BoolExpr)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("this query is not allowed on incrementally maintainable materialized view"), - errhint("sublink only supports simple conditions with EXISTS clause in WHERE clause"))); - - context->in_jointree = true; - context->is_simple_jointree = true; + context->allow_exists = true; check_ivm_restriction_walker(from->quals, context); - context->is_simple_jointree = false; - context->in_jointree = false; break; } case T_JoinExpr: @@ -1086,20 +1068,51 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) case T_BoolExpr: { BoolExpr *expr = (BoolExpr *) node; - expression_tree_walker((Node *) expr->args, check_ivm_restriction_walker, (void *) context); + BoolExprType type = ((BoolExpr *) node)->boolop; + ListCell *lc; + + switch (type) + { + case AND_EXPR: + foreach(lc, expr->args) + { + Node *opnode = (Node *) lfirst(lc); + + /* + * EXIEST is allowed under AND expression only if it is + * directly under WHERE. + */ + if (allow_exists) + context->allow_exists = true; + check_ivm_restriction_walker(opnode, context); + } + break; + case OR_EXPR: + case NOT_EXPR: + if (checkExprHasSubLink(node)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("this query is not allowed on incrementally maintainable materialized view"), + errhint("OR or NOT conditions and EXISTS condition can not be used together"))); + + expression_tree_walker((Node *) expr->args, check_ivm_restriction_walker, (void *) context); + break; + } break; } case T_SubLink: { - /* Currently, EXISTS clause is supported only */ Query *subselect; SubLink *sublink = (SubLink *) node; - if (!context->in_jointree || sublink->subLinkType != EXISTS_SUBLINK) + + /* Only EXISTS clause is supported if it is directly under WHERE */ + if (!allow_exists || sublink->subLinkType != EXISTS_SUBLINK) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("this query is not allowed on incrementally maintainable materialized view"), errhint("sublink only supports subquery with EXISTS clause in WHERE clause"))); - if (context->sublevels_up > 0 || (context->in_jointree && !context->is_simple_jointree)) + + if (context->sublevels_up > 0) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("nested sublink is not supported on incrementally maintainable materialized view"))); @@ -1119,8 +1132,6 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) break; } default: - // not a simple query, if a condition don't match these - context->is_simple_jointree = false; expression_tree_walker(node, check_ivm_restriction_walker, (void *) context); break; } diff --git a/expected/pg_ivm.out b/expected/pg_ivm.out index 66a5ba6..33ba313 100644 --- a/expected/pg_ivm.out +++ b/expected/pg_ivm.out @@ -876,7 +876,7 @@ ERROR: this query is not allowed on incrementally maintainable materialized vie HINT: targetlist must contain vars that are referred to in EXISTS subquery SELECT create_immv('mv_ivm_subquery', 'SELECT a.i,a.j FROM mv_base_a a WHERE EXISTS(SELECT 1 FROM mv_base_b b WHERE a.i = b.i) OR a.i > 2'); ERROR: this query is not allowed on incrementally maintainable materialized view -HINT: OR or NOT conditions and EXISTS condition are not used together +HINT: OR or NOT conditions and EXISTS condition can not be used together SELECT create_immv('mv_ivm_subquery', 'SELECT a.j FROM mv_base_a a WHERE EXISTS(SELECT 1 FROM mv_base_a a2 WHERE EXISTS(SELECT 1 FROM mv_base_b b WHERE a2.i = b.i))'); ERROR: nested sublink is not supported on incrementally maintainable materialized view SELECT create_immv('mv_ivm_subquery', 'SELECT EXISTS(SELECT 1 from mv_base_b) FROM mv_base_a a'); @@ -884,15 +884,16 @@ ERROR: this query is not allowed on incrementally maintainable materialized vie HINT: sublink only supports subquery with EXISTS clause in WHERE clause SELECT create_immv('mv_ivm_subquery', 'SELECT false OR EXISTS(SELECT 1 FROM mv_base_a) FROM mv_base_b'); ERROR: this query is not allowed on incrementally maintainable materialized view -HINT: sublink only supports subquery with EXISTS clause in WHERE clause +HINT: OR or NOT conditions and EXISTS condition can not be used together SELECT create_immv('mv_ivm_subquery', 'SELECT * FROM generate_series(1, CASE EXISTS(SELECT 1 FROM mv_base_a) WHEN true THEN 100 ELSE 10 END), mv_base_b'); ERROR: this query is not allowed on incrementally maintainable materialized view HINT: sublink only supports subquery with EXISTS clause in WHERE clause SELECT create_immv('mv_ivm_subquery', 'SELECT * FROM mv_base_a a WHERE CASE EXISTS(SELECT 1 FROM mv_base_b b WHERE a.i = b.i) WHEN true THEN false ELSE true END'); ERROR: this query is not allowed on incrementally maintainable materialized view -HINT: sublink only supports simple conditions with EXISTS clause in WHERE clause +HINT: sublink only supports subquery with EXISTS clause in WHERE clause SELECT create_immv('mv_ivm_subquery', 'SELECT * FROM mv_base_a a WHERE true and CASE EXISTS(SELECT 1 FROM mv_base_b b WHERE a.i = b.i) WHEN true THEN false ELSE true END'); -ERROR: nested sublink is not supported on incrementally maintainable materialized view +ERROR: this query is not allowed on incrementally maintainable materialized view +HINT: sublink only supports subquery with EXISTS clause in WHERE clause -- support join subquery in FROM clause BEGIN; SELECT create_immv('mv_ivm_join_subquery', 'SELECT i, j, k FROM ( SELECT i, a.j, b.k FROM mv_base_b b INNER JOIN mv_base_a a USING(i)) tmp'); diff --git a/matview.c b/matview.c index d12041e..5ca8526 100644 --- a/matview.c +++ b/matview.c @@ -1614,35 +1614,25 @@ rewrite_exists_subquery_walker(Query *query, Node *node, int *count) } case T_BoolExpr: { - BoolExprType type; + BoolExprType type = ((BoolExpr *) node)->boolop; - type = ((BoolExpr *) node)->boolop; - switch (type) + if (type == AND_EXPR) { ListCell *lc; - case AND_EXPR: - foreach(lc, ((BoolExpr *)node)->args) - { - /* If simple EXISTS subquery is used, rewrite LATERAL subquery */ - Node *opnode = (Node *)lfirst(lc); - query = rewrite_exists_subquery_walker(query, opnode, count); - /* - * overwrite SubLink node to true condition if it is contained in AND_EXPR. - * EXISTS clause have already overwritten to LATERAL, so original EXISTS clause - * is not necessory. - */ - if (IsA(opnode, SubLink)) - lfirst(lc) = makeConst(BOOLOID, -1, InvalidOid, sizeof(bool), BoolGetDatum(true), false, true); - } - break; - case OR_EXPR: - case NOT_EXPR: - if (checkExprHasSubLink(node)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("this query is not allowed on incrementally maintainable materialized view"), - errhint("OR or NOT conditions and EXISTS condition are not used together"))); - break; + foreach(lc, ((BoolExpr *) node)->args) + { + /* If simple EXISTS subquery is used, rewrite LATERAL subquery */ + Node *opnode = (Node *) lfirst(lc); + query = rewrite_exists_subquery_walker(query, opnode, count); + + /* + * overwrite SubLink node to true condition if it is contained in AND_EXPR. + * EXISTS clause have already overwritten to LATERAL, so original EXISTS clause + * is not necessory. + */ + if (IsA(opnode, SubLink)) + lfirst(lc) = makeConst(BOOLOID, -1, InvalidOid, sizeof(bool), BoolGetDatum(true), false, true); + } } break; } -- 2.25.1 ```

0002-Some-code-cleaning-and-comments-improvement.patch

```diff From c0b5dec3e60397a4345372d17f2d8900c6e1b596 Mon Sep 17 00:00:00 2001 From: Yugo Nagata Date: Tue, 27 Feb 2024 10:39:24 +0900 Subject: [PATCH 2/2] Some code cleaning and comments improvement --- createas.c | 35 ++++++++++++++++++++--------------- matview.c | 15 ++++++++------- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/createas.c b/createas.c index e928023..a21598e 100644 --- a/createas.c +++ b/createas.c @@ -465,7 +465,7 @@ makeIvmAggColumn(ParseState *pstate, Aggref *aggref, char *resname, AttrNumber * /* Make a Func with a dummy arg, and then override this by the original agg's args. */ node = ParseFuncOrColumn(pstate, fn->funcname, list_make1(dmy_arg), NULL, fn, false, -1); - ((Aggref *)node)->args = aggref->args; + ((Aggref *) node)->args = aggref->args; tle_count = makeTargetEntry((Expr *) node, *next_resno, @@ -501,7 +501,7 @@ makeIvmAggColumn(ParseState *pstate, Aggref *aggref, char *resname, AttrNumber * /* Make a Func with dummy args, and then override this by the original agg's args. */ node = ParseFuncOrColumn(pstate, fn->funcname, dmy_args, NULL, fn, false, -1); - ((Aggref *)node)->args = aggref->args; + ((Aggref *) node)->args = aggref->args; tle_count = makeTargetEntry((Expr *) node, *next_resno, @@ -572,7 +572,7 @@ CreateIvmTriggersOnBaseTablesRecurse(Query *qry, Node *node, Oid matviewOid, Query *query = (Query *) node; ListCell *lc; - CreateIvmTriggersOnBaseTablesRecurse(qry, (Node *)query->jointree, matviewOid, relids, ex_lock); + CreateIvmTriggersOnBaseTablesRecurse(qry, (Node *) query->jointree, matviewOid, relids, ex_lock); foreach(lc, query->cteList) { CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc); @@ -604,7 +604,7 @@ CreateIvmTriggersOnBaseTablesRecurse(Query *qry, Node *node, Oid matviewOid, { Query *subquery = rte->subquery; Assert(rte->subquery != NULL); - CreateIvmTriggersOnBaseTablesRecurse(subquery, (Node *)subquery, matviewOid, relids, ex_lock); + CreateIvmTriggersOnBaseTablesRecurse(subquery, (Node *) subquery, matviewOid, relids, ex_lock); } } break; @@ -767,7 +767,7 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) { case T_Query: { - Query *qry = (Query *)node; + Query *qry = (Query *) node; ListCell *lc; List *vars; @@ -823,7 +823,7 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) if (IsA(lfirst(lc), Var)) { Var *var = (Var *) lfirst(lc); - /* if system column, return error */ + /* if the view has a system column, raise an error */ if (var->varattno < 0) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -831,7 +831,7 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) } } - /* check if type in the top target list had an equality operator */ + /* check that each type in the target list has an equality operator */ if (context->sublevels_up == 0) { foreach(lc, qry->targetList) @@ -902,6 +902,7 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("VALUES is not supported on incrementally maintainable materialized view"))); + if (rte->relkind == RELKIND_RELATION && isImmv(rte->relid)) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -920,9 +921,11 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) /* * additional restriction checks for exists subquery * - * If the query has any EXISTS clauses and columns in them refer to - * columns in tables in the output query, those columns must be - * included in the target list. + * If the query has an EXISTS subquery and columns of a table in + * the outer query are used in the EXISTS subquery, those columns + * must be included in the target list. These columns are required + * to identify tuples in the view to be affected by modification + * of tables in the EXISTS subquery. */ if (context->exists_qual_vars != NIL && context->sublevels_up == 0) { @@ -965,7 +968,7 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("WITH query name %s is not supported on incrementally maintainable materialized view", cte->ctename))); - /* + /* * When a table in a unreferenced CTE is TRUNCATEd, the contents of the * IMMV is not affected so it must not be truncated. For confirming it * at the maintenance time, we have to check if the modified table used @@ -986,12 +989,13 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) } case T_TargetEntry: { - TargetEntry *tle = (TargetEntry *)node; + TargetEntry *tle = (TargetEntry *) node; if (isIvmName(tle->resname)) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("column name %s is not supported on incrementally maintainable materialized view", tle->resname))); + if (context->has_agg && !IsA(tle->expr, Aggref) && contain_aggs_of_level((Node *) tle->expr, 0)) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -1015,7 +1019,7 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) } case T_JoinExpr: { - JoinExpr *joinexpr = (JoinExpr *)node; + JoinExpr *joinexpr = (JoinExpr *) node; if (joinexpr->jointype > JOIN_INNER) ereport(ERROR, @@ -1117,7 +1121,8 @@ check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *context) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("nested sublink is not supported on incrementally maintainable materialized view"))); - subselect = (Query *)sublink->subselect; + subselect = (Query *) sublink->subselect; + /* raise ERROR if the sublink has CTE */ if (subselect->cteList) ereport(ERROR, @@ -1647,7 +1652,7 @@ get_primary_key_attnos_from_query(Query *query, List **constraintList) i = 1; foreach(lc, key_attnos_list) { - Bitmapset *bms = (Bitmapset *)lfirst(lc); + Bitmapset *bms = (Bitmapset *) lfirst(lc); if (!bms_is_empty(bms) && bms_is_member(i, rels_in_from)) return NULL; i++; diff --git a/matview.c b/matview.c index 5ca8526..6b8bd92 100644 --- a/matview.c +++ b/matview.c @@ -1606,7 +1606,8 @@ rewrite_exists_subquery_walker(Query *query, Node *node, int *count) if (fromexpr->quals != NULL) { query = rewrite_exists_subquery_walker(query, fromexpr->quals, count); - /* drop subquery in WHERE clause */ + + /* drop WHERE clause when it has only one EXISTS */ if (IsA(fromexpr->quals, SubLink)) fromexpr->quals = NULL; } @@ -1691,7 +1692,7 @@ rewrite_exists_subquery_walker(Query *query, Node *node, int *count) /* assume the new RTE is at the end */ rtr = makeNode(RangeTblRef); rtr->rtindex = list_length(query->rtable); - ((FromExpr *)query->jointree)->fromlist = lappend(((FromExpr *)query->jointree)->fromlist, rtr); + ((FromExpr *) query->jointree)->fromlist = lappend(((FromExpr *) query->jointree)->fromlist, rtr); /* * EXISTS condition is converted to HAVING count(*) > 0. @@ -1699,13 +1700,13 @@ rewrite_exists_subquery_walker(Query *query, Node *node, int *count) */ opId = OpernameGetOprid(list_make2(makeString("pg_catalog"), makeString(">")), INT8OID, INT4OID); opexpr = make_opclause(opId, BOOLOID, false, - (Expr *)fn_node, - (Expr *)makeConst(INT4OID, -1, InvalidOid, sizeof(int32), Int32GetDatum(0), false, true), + (Expr *) fn_node, + (Expr *) makeConst(INT4OID, -1, InvalidOid, sizeof(int32), Int32GetDatum(0), false, true), InvalidOid, InvalidOid); fix_opfuncids((Node *) opexpr); query->hasSubLinks = false; - subselect->havingQual = (Node *)opexpr; + subselect->havingQual = (Node *) opexpr; (*count)++; break; } @@ -1728,7 +1729,7 @@ rewrite_exists_subquery_walker(Query *query, Node *node, int *count) * SELECT 1, COUNT(*) AS __ivm_exists_count_0__ * FROM t2 * WHERE t1.key = t2.key - * HAVING __ivm_exists_count_0__ > 0) AS ex + * HAVING COUNT(*) > 0) AS ex */ Query * rewrite_query_for_exists_subquery(Query *query) @@ -1740,7 +1741,7 @@ rewrite_query_for_exists_subquery(Query *query) errmsg("this query is not allowed on incrementally maintainable materialized view"), errhint("aggregate function and EXISTS condition are not supported at the same time"))); - return rewrite_exists_subquery_walker(query, (Node *)query, &count); + return rewrite_exists_subquery_walker(query, (Node *) query, &count); } /* -- 2.25.1 ```

[thoshiai]

Thanks for review and suggestion. I apply your patches!

[yugo-n]

Could you fix the conflict on the regression test?