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 <zyu@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 <nagata@sraoss.co.jp>