Hello Zhihong Yu, Thank you for your suggestion!
I am sorry for late replay. I'll fix them and submit the updated patch soon. On Sat, 7 Aug 2021 00:52:24 -0700 Zhihong Yu <z...@yugabyte.com> wrote: > > Hi, > > For v23-0007-Add-Incremental-View-Maintenance-support.patch : > > > > bq. In this implementation, AFTER triggers are used to collecting > > tuplestores > > > > 'to collecting' -> to collect > > > > bq. are contained in a old transition table. > > > > 'a old' -> an old > > > > bq. updates of more than one base tables > > > > one base tables -> one base table I'll fix them. > > bq. DISTINCT and tuple duplicates in views are supported > > > > Since distinct and duplicate have opposite meanings, it would be better to > > rephrase the above sentence. I'll rewrite it to "Incrementally Maintainable Materialized Views (IMMV) can contain duplicated tuples. Also, DISTINCT clause is supported. " > > bq. The value in__ivm_count__ is updated > > > > I searched the patch for in__ivm_count__ - there was no (second) match. I > > think there should be a space between in and underscore. Yes, the space was missing. > > +static void CreateIvmTriggersOnBaseTables_recurse(Query *qry, Node *node, > > Oid matviewOid, Relids *relids, bool ex_lock); > > > > nit: long line. please wrap. OK. > > > > + if (rewritten->distinctClause) > > + rewritten->groupClause = transformDistinctClause(NULL, > > &rewritten->targetList, rewritten->sortClause, false); > > + > > + /* Add count(*) for counting distinct tuples in views */ > > + if (rewritten->distinctClause) > > > > It seems the body of the two if statements can be combined into one. Ok. > > + CreateIvmTriggersOnBaseTables_recurse(qry, (Node *)qry, matviewOid, > &relids, ex_lock); > > Looking at existing recursive functions, e.g. > > src/backend/executor/execPartition.c:find_matching_subplans_recurse(PartitionPruningData > *prunedata, > > the letters in the function name are all lower case. I think following the > convention would be nice. Ok. I'll rename this to CreateIvmTriggersOnBaseTablesRecurse since I found DeadLockCheckRecurse, transformExprRecurse, and so on. > > + if (rte->rtekind == RTE_RELATION) > + { > + if (!bms_is_member(rte->relid, *relids)) > > The conditions for the two if statements can be combined (saving some > indentation). Yes. I'll fix. > + check_stack_depth(); > + > + if (node == NULL) > + return false; > > It seems the node check can be placed ahead of the stack depth check. OK. > + * CreateindexOnIMMV > > CreateindexOnIMMV -> CreateIndexOnIMMV > > + (errmsg("could not create an index on materialized view > \"%s\" automatically", > > It would be nice to mention the reason is the lack of primary key. > > + /* create no index, just notice that an appropriate index is > necessary for efficient, IVM */ > > for efficient -> for efficiency. I'll fix them. Thanks. Regards, Yugo Nagata -- Yugo NAGATA <nag...@sraoss.co.jp>