Re: Implementing Incremental View Maintenance

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(at)yugabyte(dot)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(at)sraoss(dot)co(dot)jp>