On Wed, 16 Feb 2022 22:34:18 +0800
huyajun <hu_yajun@qq.com> wrote:
> Hi, Nagata-san
> I am very interested in IMMV and read your patch but have some comments in
v25-0007-Add-Incremental-View-Maintenance-support.patchand want to discuss with you.
Thank you for your review!
>
> + /* For IMMV, we need to rewrite matview query */
> + query = rewriteQueryForIMMV(query, into->colNames);
> + query_immv = copyObject(query);
>
> /* Create triggers on incremental maintainable materialized view */
> + Assert(query_immv != NULL);
> + CreateIvmTriggersOnBaseTables(query_immv, matviewOid, true);
> 1. Do we need copy query?Is it okay that CreateIvmTriggersOnBaseTables directly use (Query *) into->viewQuery
insteadof query_immv like CreateIndexOnIMMV? It seems only planner may change query, but it shouldn't affect us
findingthe correct base table in CreateIvmTriggersOnBaseTables .
The copy to query_immv was necessary for supporting sub-queries in the view
definition. However, we excluded the fueature from the current patch to reduce
the patch size, so it would be unnecessary. I'll fix it.
>
> +void
> +CreateIndexOnIMMV(Query *query, Relation matviewRel)
> +{
> + Query *qry = (Query *) copyObject(query);
> 2. Also, is it okay to not copy query in CreateIndexOnIMMV? It seems we only read query in CreateIndexOnIMMV.
This was also necessary for supporting CTEs, but unnecessary in the current
patch, so I'll fix it, too.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>